| Message ID | 20240408145454.718047-6-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/8] xfs: fix error returns from xfs_bmapi_write | expand |
On Mon, Apr 08, 2024 at 04:54:51PM +0200, Christoph Hellwig wrote: > xfs_bmapi_allocate currently overwrites offset and len when converting > delayed allocations, and duplicates the length cap done for non-delalloc > allocations. Move all that logic into the callers to avoid duplication > and to make the calling conventions more obvious. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/libxfs/xfs_bmap.c | 32 ++++++++++++++++++-------------- > 1 file changed, 18 insertions(+), 14 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index f2e934c2fb423c..aa182937de4641 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -4194,21 +4194,11 @@ xfs_bmapi_allocate( > int error; > > ASSERT(bma->length > 0); > + ASSERT(bma->length <= XFS_MAX_BMBT_EXTLEN); > > - /* > - * For the wasdelay case, we could also just allocate the stuff asked > - * for in this bmap call but that wouldn't be as good. > - */ > if (bma->wasdel) { > - bma->length = (xfs_extlen_t)bma->got.br_blockcount; > - bma->offset = bma->got.br_startoff; > if (!xfs_iext_peek_prev_extent(ifp, &bma->icur, &bma->prev)) > bma->prev.br_startoff = NULLFILEOFF; > - } else { > - bma->length = XFS_FILBLKS_MIN(bma->length, XFS_MAX_BMBT_EXTLEN); > - if (!bma->eof) > - bma->length = XFS_FILBLKS_MIN(bma->length, > - bma->got.br_startoff - bma->offset); > } > > if (bma->flags & XFS_BMAPI_CONTIG) > @@ -4542,6 +4532,15 @@ xfs_bmapi_write( > */ > bma.length = XFS_FILBLKS_MIN(len, XFS_MAX_BMBT_EXTLEN); > > + if (wasdelay) { > + bma.offset = bma.got.br_startoff; > + bma.length = bma.got.br_blockcount; This read funny since we'd previously set bma.{offset,length} above, but I guess that preserves the "convert all the delalloc" behavior; and you turn it off in patch 8, right? --D > + } else { > + if (!eof) > + bma.length = XFS_FILBLKS_MIN(bma.length, > + bma.got.br_startoff - bno); > + } > + > ASSERT(bma.length > 0); > error = xfs_bmapi_allocate(&bma); > if (error) { > @@ -4694,11 +4693,16 @@ xfs_bmapi_convert_delalloc( > bma.tp = tp; > bma.ip = ip; > bma.wasdel = true; > - bma.offset = bma.got.br_startoff; > - bma.length = max_t(xfs_filblks_t, bma.got.br_blockcount, > - XFS_MAX_BMBT_EXTLEN); > bma.minleft = xfs_bmapi_minleft(tp, ip, whichfork); > > + /* > + * Always allocate convert from the start of the delalloc extent even if > + * that is outside the passed in range to create large contiguous > + * extents on disk. > + */ > + bma.offset = bma.got.br_startoff; > + bma.length = bma.got.br_blockcount; > + > /* > * When we're converting the delalloc reservations backing dirty pages > * in the page cache, we must be careful about how we create the new > -- > 2.39.2 > >
On Tue, Apr 09, 2024 at 04:20:44PM -0700, Darrick J. Wong wrote: > > @@ -4542,6 +4532,15 @@ xfs_bmapi_write( > > */ > > bma.length = XFS_FILBLKS_MIN(len, XFS_MAX_BMBT_EXTLEN); > > > > + if (wasdelay) { > > + bma.offset = bma.got.br_startoff; > > + bma.length = bma.got.br_blockcount; > > This read funny since we'd previously set bma.{offset,length} above, but > I guess that preserves the "convert all the delalloc" behavior; and you > turn it off in patch 8, right? Yes. That being said I should just move the assignments into the other branch here to make things more obvious.
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index f2e934c2fb423c..aa182937de4641 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -4194,21 +4194,11 @@ xfs_bmapi_allocate( int error; ASSERT(bma->length > 0); + ASSERT(bma->length <= XFS_MAX_BMBT_EXTLEN); - /* - * For the wasdelay case, we could also just allocate the stuff asked - * for in this bmap call but that wouldn't be as good. - */ if (bma->wasdel) { - bma->length = (xfs_extlen_t)bma->got.br_blockcount; - bma->offset = bma->got.br_startoff; if (!xfs_iext_peek_prev_extent(ifp, &bma->icur, &bma->prev)) bma->prev.br_startoff = NULLFILEOFF; - } else { - bma->length = XFS_FILBLKS_MIN(bma->length, XFS_MAX_BMBT_EXTLEN); - if (!bma->eof) - bma->length = XFS_FILBLKS_MIN(bma->length, - bma->got.br_startoff - bma->offset); } if (bma->flags & XFS_BMAPI_CONTIG) @@ -4542,6 +4532,15 @@ xfs_bmapi_write( */ bma.length = XFS_FILBLKS_MIN(len, XFS_MAX_BMBT_EXTLEN); + if (wasdelay) { + bma.offset = bma.got.br_startoff; + bma.length = bma.got.br_blockcount; + } else { + if (!eof) + bma.length = XFS_FILBLKS_MIN(bma.length, + bma.got.br_startoff - bno); + } + ASSERT(bma.length > 0); error = xfs_bmapi_allocate(&bma); if (error) { @@ -4694,11 +4693,16 @@ xfs_bmapi_convert_delalloc( bma.tp = tp; bma.ip = ip; bma.wasdel = true; - bma.offset = bma.got.br_startoff; - bma.length = max_t(xfs_filblks_t, bma.got.br_blockcount, - XFS_MAX_BMBT_EXTLEN); bma.minleft = xfs_bmapi_minleft(tp, ip, whichfork); + /* + * Always allocate convert from the start of the delalloc extent even if + * that is outside the passed in range to create large contiguous + * extents on disk. + */ + bma.offset = bma.got.br_startoff; + bma.length = bma.got.br_blockcount; + /* * When we're converting the delalloc reservations backing dirty pages * in the page cache, we must be careful about how we create the new
xfs_bmapi_allocate currently overwrites offset and len when converting delayed allocations, and duplicates the length cap done for non-delalloc allocations. Move all that logic into the callers to avoid duplication and to make the calling conventions more obvious. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_bmap.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)