| Message ID | faf2e8e2cb4f87f4b418b177256d8950cd8deba1.1524523682.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 24.04.2018 02:03, David Sterba wrote: > The eb length is nodesize, as initialized in __alloc_extent_buffer. > Regardless of start, we should always get the same number of pages, so > use that fact. > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/extent_io.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h > index a53009694b16..ee92c1289edd 100644 > --- a/fs/btrfs/extent_io.h > +++ b/fs/btrfs/extent_io.h > @@ -454,8 +454,7 @@ void wait_on_extent_buffer_writeback(struct extent_buffer *eb); > > static inline unsigned long num_extent_pages(u64 start, u64 len) > { > - return ((start + len + PAGE_SIZE - 1) >> PAGE_SHIFT) - > - (start >> PAGE_SHIFT); > + return len >> PAGE_SHIFT; Shouldn't this really be len + PAGE_SIZE -1 or in fact DIV_ROUND_DOWN (len, PAGE_SIZE). Because with a nodesize of 4k (and basically less than a page size) we can get into a situation where we do: 4096 >> 13 = 0 On powerpc for example we have: arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 18 arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 16 arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 14 arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 12 > } > > static inline void extent_buffer_get(struct extent_buffer *eb) > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018年04月24日 13:59, Nikolay Borisov wrote: > > > On 24.04.2018 02:03, David Sterba wrote: >> The eb length is nodesize, as initialized in __alloc_extent_buffer. >> Regardless of start, we should always get the same number of pages, so >> use that fact. >> >> Signed-off-by: David Sterba <dsterba@suse.com> >> --- >> fs/btrfs/extent_io.h | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h >> index a53009694b16..ee92c1289edd 100644 >> --- a/fs/btrfs/extent_io.h >> +++ b/fs/btrfs/extent_io.h >> @@ -454,8 +454,7 @@ void wait_on_extent_buffer_writeback(struct extent_buffer *eb); >> >> static inline unsigned long num_extent_pages(u64 start, u64 len) >> { >> - return ((start + len + PAGE_SIZE - 1) >> PAGE_SHIFT) - >> - (start >> PAGE_SHIFT); >> + return len >> PAGE_SHIFT; > > Shouldn't this really be len + PAGE_SIZE -1 or in fact DIV_ROUND_DOWN > (len, PAGE_SIZE). Because with a nodesize of 4k (and basically less than > a page size) we can get into a situation where we do: > > 4096 >> 13 = 0 > > On powerpc for example we have: > > arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 18 > arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 16 > arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 14 For such case, the fs won't be mounted as we don't have sub-pagesized nodesize support yet. So won't hit the problem. Although a WARN_ON(len < PAGE_SIZE || IS_ALIGNED(start, PAGE_SIZE)) would do no harm here. Thanks, Qu > arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 12 > > >> } >> >> static inline void extent_buffer_get(struct extent_buffer *eb) >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 24, 2018 at 02:22:15PM +0800, Qu Wenruo wrote: > > > On 2018年04月24日 13:59, Nikolay Borisov wrote: > > > > > > On 24.04.2018 02:03, David Sterba wrote: > >> The eb length is nodesize, as initialized in __alloc_extent_buffer. > >> Regardless of start, we should always get the same number of pages, so > >> use that fact. > >> > >> Signed-off-by: David Sterba <dsterba@suse.com> > >> --- > >> fs/btrfs/extent_io.h | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h > >> index a53009694b16..ee92c1289edd 100644 > >> --- a/fs/btrfs/extent_io.h > >> +++ b/fs/btrfs/extent_io.h > >> @@ -454,8 +454,7 @@ void wait_on_extent_buffer_writeback(struct extent_buffer *eb); > >> > >> static inline unsigned long num_extent_pages(u64 start, u64 len) > >> { > >> - return ((start + len + PAGE_SIZE - 1) >> PAGE_SHIFT) - > >> - (start >> PAGE_SHIFT); > >> + return len >> PAGE_SHIFT; > > > > Shouldn't this really be len + PAGE_SIZE -1 or in fact DIV_ROUND_DOWN > > (len, PAGE_SIZE). Because with a nodesize of 4k (and basically less than > > a page size) we can get into a situation where we do: > > > > 4096 >> 13 = 0 > > > > On powerpc for example we have: > > > > arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 18 > > arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 16 > > arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 14 > > For such case, the fs won't be mounted as we don't have sub-pagesized > nodesize support yet. > So won't hit the problem. > > Although a WARN_ON(len < PAGE_SIZE || IS_ALIGNED(start, PAGE_SIZE)) > would do no harm here. Such check is fine, but would be better placed in __alloc_extent_buffer, not each time we access the eb. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018年04月24日 18:29, David Sterba wrote: > On Tue, Apr 24, 2018 at 02:22:15PM +0800, Qu Wenruo wrote: >> >> >> On 2018年04月24日 13:59, Nikolay Borisov wrote: >>> >>> >>> On 24.04.2018 02:03, David Sterba wrote: >>>> The eb length is nodesize, as initialized in __alloc_extent_buffer. >>>> Regardless of start, we should always get the same number of pages, so >>>> use that fact. >>>> >>>> Signed-off-by: David Sterba <dsterba@suse.com> >>>> --- >>>> fs/btrfs/extent_io.h | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h >>>> index a53009694b16..ee92c1289edd 100644 >>>> --- a/fs/btrfs/extent_io.h >>>> +++ b/fs/btrfs/extent_io.h >>>> @@ -454,8 +454,7 @@ void wait_on_extent_buffer_writeback(struct extent_buffer *eb); >>>> >>>> static inline unsigned long num_extent_pages(u64 start, u64 len) >>>> { >>>> - return ((start + len + PAGE_SIZE - 1) >> PAGE_SHIFT) - >>>> - (start >> PAGE_SHIFT); >>>> + return len >> PAGE_SHIFT; >>> >>> Shouldn't this really be len + PAGE_SIZE -1 or in fact DIV_ROUND_DOWN >>> (len, PAGE_SIZE). Because with a nodesize of 4k (and basically less than >>> a page size) we can get into a situation where we do: >>> >>> 4096 >> 13 = >>> >>> On powerpc for example we have: >>> >>> arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 18 >>> arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 16 >>> arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 14 >> >> For such case, the fs won't be mounted as we don't have sub-pagesized >> nodesize support yet. >> So won't hit the problem. >> >> Although a WARN_ON(len < PAGE_SIZE || IS_ALIGNED(start, PAGE_SIZE)) >> would do no harm here. > > Such check is fine, but would be better placed in __alloc_extent_buffer, > not each time we access the eb. Yep, makes more sense than my initial idea. Thanks, Qu > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h index a53009694b16..ee92c1289edd 100644 --- a/fs/btrfs/extent_io.h +++ b/fs/btrfs/extent_io.h @@ -454,8 +454,7 @@ void wait_on_extent_buffer_writeback(struct extent_buffer *eb); static inline unsigned long num_extent_pages(u64 start, u64 len) { - return ((start + len + PAGE_SIZE - 1) >> PAGE_SHIFT) - - (start >> PAGE_SHIFT); + return len >> PAGE_SHIFT; } static inline void extent_buffer_get(struct extent_buffer *eb)
The eb length is nodesize, as initialized in __alloc_extent_buffer. Regardless of start, we should always get the same number of pages, so use that fact. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/extent_io.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)