| Message ID | 20210526224722.1111377-8-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: buffer bulk page allocation and cleanups | expand |
On Thu, May 27, 2021 at 08:47:19AM +1000, Dave Chinner wrote: > From: Christoph Hellwig <hch@lst.de> > > Ever since we stopped using the Linux page cache The premise of /that/ is unsettling. Um... did b_pages[] point to pagecache pages, and that's why all that offset shifting was necessary? > to back XFS buffes s/buffes/buffers/ > there is no need to take the start sector into account for > calculating the number of pages in a buffer, as the data always > start from the beginning of the buffer. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > > Signed-off-by: Christoph Hellwig <hch@lst.de> > [dgc: modified to suit this series] > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_buf.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 87151d78a0d8..1500a9c63432 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -348,14 +348,13 @@ xfs_buf_alloc_kmem( > static int > xfs_buf_alloc_pages( > struct xfs_buf *bp, > - uint page_count, > xfs_buf_flags_t flags) > { > gfp_t gfp_mask = xb_to_gfp(flags); > long filled = 0; > > /* Make sure that we have a page list */ > - bp->b_page_count = page_count; > + bp->b_page_count = DIV_ROUND_UP(BBTOB(bp->b_length), PAGE_SIZE); > if (bp->b_page_count <= XB_PAGES) { > bp->b_pages = bp->b_page_array; > } else { > @@ -409,7 +408,6 @@ xfs_buf_allocate_memory( > uint flags) > { > size_t size; > - xfs_off_t start, end; > int error; > > /* > @@ -424,11 +422,7 @@ xfs_buf_allocate_memory( > if (!error) > return 0; > } > - > - start = BBTOB(bp->b_maps[0].bm_bn) >> PAGE_SHIFT; > - end = (BBTOB(bp->b_maps[0].bm_bn + bp->b_length) + PAGE_SIZE - 1) > - >> PAGE_SHIFT; > - return xfs_buf_alloc_pages(bp, end - start, flags); > + return xfs_buf_alloc_pages(bp, flags); > } > > /* > @@ -922,7 +916,6 @@ xfs_buf_get_uncached( > int flags, > struct xfs_buf **bpp) > { > - unsigned long page_count; > int error; > struct xfs_buf *bp; > DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks); > @@ -934,8 +927,7 @@ xfs_buf_get_uncached( > if (error) > return error; > > - page_count = PAGE_ALIGN(numblks << BBSHIFT) >> PAGE_SHIFT; > - error = xfs_buf_alloc_pages(bp, page_count, flags); > + error = xfs_buf_alloc_pages(bp, flags); > if (error) > goto fail_free_buf; > > -- > 2.31.1 >
On Thu, May 27, 2021 at 04:15:44PM -0700, Darrick J. Wong wrote: > On Thu, May 27, 2021 at 08:47:19AM +1000, Dave Chinner wrote: > > From: Christoph Hellwig <hch@lst.de> > > > > Ever since we stopped using the Linux page cache > > The premise of /that/ is unsettling. Um... did b_pages[] point to > pagecache pages, and that's why all that offset shifting was necessary? Yes. So things like sector/block size < page size would work correctly. We could have multiple buffers point at different offsets in the same page... > > to back XFS buffes > > s/buffes/buffers/ > > > there is no need to take the start sector into account for > > calculating the number of pages in a buffer, as the data always > > start from the beginning of the buffer. > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> Ta! -Dave.
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 87151d78a0d8..1500a9c63432 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -348,14 +348,13 @@ xfs_buf_alloc_kmem( static int xfs_buf_alloc_pages( struct xfs_buf *bp, - uint page_count, xfs_buf_flags_t flags) { gfp_t gfp_mask = xb_to_gfp(flags); long filled = 0; /* Make sure that we have a page list */ - bp->b_page_count = page_count; + bp->b_page_count = DIV_ROUND_UP(BBTOB(bp->b_length), PAGE_SIZE); if (bp->b_page_count <= XB_PAGES) { bp->b_pages = bp->b_page_array; } else { @@ -409,7 +408,6 @@ xfs_buf_allocate_memory( uint flags) { size_t size; - xfs_off_t start, end; int error; /* @@ -424,11 +422,7 @@ xfs_buf_allocate_memory( if (!error) return 0; } - - start = BBTOB(bp->b_maps[0].bm_bn) >> PAGE_SHIFT; - end = (BBTOB(bp->b_maps[0].bm_bn + bp->b_length) + PAGE_SIZE - 1) - >> PAGE_SHIFT; - return xfs_buf_alloc_pages(bp, end - start, flags); + return xfs_buf_alloc_pages(bp, flags); } /* @@ -922,7 +916,6 @@ xfs_buf_get_uncached( int flags, struct xfs_buf **bpp) { - unsigned long page_count; int error; struct xfs_buf *bp; DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks); @@ -934,8 +927,7 @@ xfs_buf_get_uncached( if (error) return error; - page_count = PAGE_ALIGN(numblks << BBSHIFT) >> PAGE_SHIFT; - error = xfs_buf_alloc_pages(bp, page_count, flags); + error = xfs_buf_alloc_pages(bp, flags); if (error) goto fail_free_buf;