| Message ID | 20240318224715.3367463-7-david@fromorbit.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xfs: use large folios for buffers | expand |
On Tue, Mar 19, 2024 at 09:45:57AM +1100, Dave Chinner wrote: > From: Christoph Hellwig <hch@lst.de> > > With the concept of unmapped buffer gone, there is no reason to not > vmap the buffer pages directly in xfs_buf_alloc_folios. "..no reason to not map the buffer pages..."? I say that mostly because the first dumb thought I had was "wait, we're going to vm_map_ram everything now??" which of course is not true. > [dchinner: port to folio-enabled buffers.] > > Signed-off-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Dave Chinner <dchinner@redhat.com> With that changed, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_buf.c | 37 ++++++------------------------------- > 1 file changed, 6 insertions(+), 31 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 2cd3671f3ce3..a77e2d8c8107 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -469,18 +469,7 @@ xfs_buf_alloc_folios( > XFS_STATS_INC(bp->b_mount, xb_page_retries); > memalloc_retry_wait(gfp_mask); > } > - return 0; > -} > > -/* > - * Map buffer into kernel address-space if necessary. > - */ > -STATIC int > -_xfs_buf_map_folios( > - struct xfs_buf *bp, > - xfs_buf_flags_t flags) > -{ > - ASSERT(bp->b_flags & _XBF_FOLIOS); > if (bp->b_folio_count == 1) { > /* A single folio buffer is always mappable */ > bp->b_addr = folio_address(bp->b_folios[0]); > @@ -513,8 +502,13 @@ _xfs_buf_map_folios( > } while (retried++ <= 1); > memalloc_nofs_restore(nofs_flag); > > - if (!bp->b_addr) > + if (!bp->b_addr) { > + xfs_warn_ratelimited(bp->b_mount, > + "%s: failed to map %u folios", __func__, > + bp->b_folio_count); > + xfs_buf_free_folios(bp); > return -ENOMEM; > + } > } > > return 0; > @@ -816,18 +810,6 @@ xfs_buf_get_map( > xfs_perag_put(pag); > } > > - /* We do not hold a perag reference anymore. */ > - if (!bp->b_addr) { > - error = _xfs_buf_map_folios(bp, flags); > - if (unlikely(error)) { > - xfs_warn_ratelimited(btp->bt_mount, > - "%s: failed to map %u folios", __func__, > - bp->b_folio_count); > - xfs_buf_relse(bp); > - return error; > - } > - } > - > /* > * Clear b_error if this is a lookup from a caller that doesn't expect > * valid data to be found in the buffer. > @@ -1068,13 +1050,6 @@ xfs_buf_get_uncached( > if (error) > goto fail_free_buf; > > - error = _xfs_buf_map_folios(bp, 0); > - if (unlikely(error)) { > - xfs_warn(target->bt_mount, > - "%s: failed to map folios", __func__); > - goto fail_free_buf; > - } > - > trace_xfs_buf_get_uncached(bp, _RET_IP_); > *bpp = bp; > return 0; > -- > 2.43.0 > >
On Tue, Mar 19, 2024 at 10:34:13AM -0700, Darrick J. Wong wrote: > On Tue, Mar 19, 2024 at 09:45:57AM +1100, Dave Chinner wrote: > > From: Christoph Hellwig <hch@lst.de> > > > > With the concept of unmapped buffer gone, there is no reason to not > > vmap the buffer pages directly in xfs_buf_alloc_folios. > > "..no reason to not map the buffer pages..."? or maybe "..no good reason to not map the buffer pages..."
On Tue, Mar 19, 2024 at 02:32:48PM -0700, Christoph Hellwig wrote: > On Tue, Mar 19, 2024 at 10:34:13AM -0700, Darrick J. Wong wrote: > > On Tue, Mar 19, 2024 at 09:45:57AM +1100, Dave Chinner wrote: > > > From: Christoph Hellwig <hch@lst.de> > > > > > > With the concept of unmapped buffer gone, there is no reason to not > > > vmap the buffer pages directly in xfs_buf_alloc_folios. > > > > "..no reason to not map the buffer pages..."? > > or maybe > > "..no good reason to not map the buffer pages..." We might as well fix the split infinitive as well: "...no good reason not to map the buffer pages..." --D
On Tue, Mar 19, 2024 at 02:39:19PM -0700, Darrick J. Wong wrote: > We might as well fix the split infinitive as well: > > "...no good reason not to map the buffer pages..." fine.
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 2cd3671f3ce3..a77e2d8c8107 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -469,18 +469,7 @@ xfs_buf_alloc_folios( XFS_STATS_INC(bp->b_mount, xb_page_retries); memalloc_retry_wait(gfp_mask); } - return 0; -} -/* - * Map buffer into kernel address-space if necessary. - */ -STATIC int -_xfs_buf_map_folios( - struct xfs_buf *bp, - xfs_buf_flags_t flags) -{ - ASSERT(bp->b_flags & _XBF_FOLIOS); if (bp->b_folio_count == 1) { /* A single folio buffer is always mappable */ bp->b_addr = folio_address(bp->b_folios[0]); @@ -513,8 +502,13 @@ _xfs_buf_map_folios( } while (retried++ <= 1); memalloc_nofs_restore(nofs_flag); - if (!bp->b_addr) + if (!bp->b_addr) { + xfs_warn_ratelimited(bp->b_mount, + "%s: failed to map %u folios", __func__, + bp->b_folio_count); + xfs_buf_free_folios(bp); return -ENOMEM; + } } return 0; @@ -816,18 +810,6 @@ xfs_buf_get_map( xfs_perag_put(pag); } - /* We do not hold a perag reference anymore. */ - if (!bp->b_addr) { - error = _xfs_buf_map_folios(bp, flags); - if (unlikely(error)) { - xfs_warn_ratelimited(btp->bt_mount, - "%s: failed to map %u folios", __func__, - bp->b_folio_count); - xfs_buf_relse(bp); - return error; - } - } - /* * Clear b_error if this is a lookup from a caller that doesn't expect * valid data to be found in the buffer. @@ -1068,13 +1050,6 @@ xfs_buf_get_uncached( if (error) goto fail_free_buf; - error = _xfs_buf_map_folios(bp, 0); - if (unlikely(error)) { - xfs_warn(target->bt_mount, - "%s: failed to map folios", __func__); - goto fail_free_buf; - } - trace_xfs_buf_get_uncached(bp, _RET_IP_); *bpp = bp; return 0;