| Message ID | 20240827065123.1762168-5-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] block: remove checks for FALLOC_FL_NO_HIDE_STALE | expand |
On Tue, Aug 27, 2024 at 08:50:48AM +0200, Christoph Hellwig wrote: > Call xfs_flush_unmap_range from xfs_free_file_space so that > xfs_file_fallocate doesn't have to predict which mode will call it. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Hmm. I /think/ it's ok to shift the xfs_flush_unmap_range after the file_modified and some of the other EINVAL bailouts that can happen before xfs_free_file_space gets called. Effectively that means that we can fail faster now? :) Later on this means that the cow-around code that the rtreflink patchset introduces will also get flushed to disk before we start collapsing/zeroing/punching. AFAICT that should be fine. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_bmap_util.c | 8 ++++++++ > fs/xfs/xfs_file.c | 21 --------------------- > 2 files changed, 8 insertions(+), 21 deletions(-) > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > index fe2e2c93097550..187a0dbda24fc4 100644 > --- a/fs/xfs/xfs_bmap_util.c > +++ b/fs/xfs/xfs_bmap_util.c > @@ -848,6 +848,14 @@ xfs_free_file_space( > if (len <= 0) /* if nothing being freed */ > return 0; > > + /* > + * Now AIO and DIO has drained we flush and (if necessary) invalidate > + * the cached range over the first operation we are about to run. > + */ > + error = xfs_flush_unmap_range(ip, offset, len); > + if (error) > + return error; > + > startoffset_fsb = XFS_B_TO_FSB(mp, offset); > endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len); > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 4cdc54dc96862e..5b9e49da06013c 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -890,27 +890,6 @@ xfs_file_fallocate( > */ > inode_dio_wait(inode); > > - /* > - * Now AIO and DIO has drained we flush and (if necessary) invalidate > - * the cached range over the first operation we are about to run. > - * > - * We care about zero and collapse here because they both run a hole > - * punch over the range first. Because that can zero data, and the range > - * of invalidation for the shift operations is much larger, we still do > - * the required flush for collapse in xfs_prepare_shift(). > - * > - * Insert has the same range requirements as collapse, and we extend the > - * file first which can zero data. Hence insert has the same > - * flush/invalidate requirements as collapse and so they are both > - * handled at the right time by xfs_prepare_shift(). > - */ > - if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE | > - FALLOC_FL_COLLAPSE_RANGE)) { > - error = xfs_flush_unmap_range(ip, offset, len); > - if (error) > - goto out_unlock; > - } > - > error = file_modified(file); > if (error) > goto out_unlock; > -- > 2.43.0 > >
On Tue, Aug 27, 2024 at 09:03:23AM -0700, Darrick J. Wong wrote: > On Tue, Aug 27, 2024 at 08:50:48AM +0200, Christoph Hellwig wrote: > > Call xfs_flush_unmap_range from xfs_free_file_space so that > > xfs_file_fallocate doesn't have to predict which mode will call it. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Hmm. I /think/ it's ok to shift the xfs_flush_unmap_range after the > file_modified and some of the other EINVAL bailouts that can happen > before xfs_free_file_space gets called. Effectively that means that we > can fail faster now? :) Yes, failing faster has always been my personal benchmark :)
On Tue, Aug 27, 2024 at 08:50:48AM +0200, Christoph Hellwig wrote: > Call xfs_flush_unmap_range from xfs_free_file_space so that > xfs_file_fallocate doesn't have to predict which mode will call it. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_bmap_util.c | 8 ++++++++ > fs/xfs/xfs_file.c | 21 --------------------- > 2 files changed, 8 insertions(+), 21 deletions(-) > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > index fe2e2c93097550..187a0dbda24fc4 100644 > --- a/fs/xfs/xfs_bmap_util.c > +++ b/fs/xfs/xfs_bmap_util.c > @@ -848,6 +848,14 @@ xfs_free_file_space( > if (len <= 0) /* if nothing being freed */ > return 0; > > + /* > + * Now AIO and DIO has drained we flush and (if necessary) invalidate > + * the cached range over the first operation we are about to run. > + */ > + error = xfs_flush_unmap_range(ip, offset, len); > + if (error) > + return error; > + > startoffset_fsb = XFS_B_TO_FSB(mp, offset); > endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len); Ok, so if we ever change the zeroing implementation to not punch a hole first, we're going to have to make sure we add this to whatever new zeroing implementation we have. Can you leave a comment in the FALLOC_FL_ZERO_RANGE implementation code that notes it currently relies on the xfs_flush_unmap_range() in xfs_free_file_space() for correct operation? -Dave.
On Thu, Aug 29, 2024 at 01:03:31PM +1000, Dave Chinner wrote: > > @@ -848,6 +848,14 @@ xfs_free_file_space( > > if (len <= 0) /* if nothing being freed */ > > return 0; > > > > + /* > > + * Now AIO and DIO has drained we flush and (if necessary) invalidate > > + * the cached range over the first operation we are about to run. > > + */ > > + error = xfs_flush_unmap_range(ip, offset, len); > > + if (error) > > + return error; > > + > > startoffset_fsb = XFS_B_TO_FSB(mp, offset); > > endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len); > > Ok, so if we ever change the zeroing implementation to not punch a > hole first, we're going to have to make sure we add this to whatever > new zeroing implementation we have. > > Can you leave a comment in the FALLOC_FL_ZERO_RANGE implementation > code that notes it currently relies on the xfs_flush_unmap_range() > in xfs_free_file_space() for correct operation? Sure.
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index fe2e2c93097550..187a0dbda24fc4 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -848,6 +848,14 @@ xfs_free_file_space( if (len <= 0) /* if nothing being freed */ return 0; + /* + * Now AIO and DIO has drained we flush and (if necessary) invalidate + * the cached range over the first operation we are about to run. + */ + error = xfs_flush_unmap_range(ip, offset, len); + if (error) + return error; + startoffset_fsb = XFS_B_TO_FSB(mp, offset); endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len); diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 4cdc54dc96862e..5b9e49da06013c 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -890,27 +890,6 @@ xfs_file_fallocate( */ inode_dio_wait(inode); - /* - * Now AIO and DIO has drained we flush and (if necessary) invalidate - * the cached range over the first operation we are about to run. - * - * We care about zero and collapse here because they both run a hole - * punch over the range first. Because that can zero data, and the range - * of invalidation for the shift operations is much larger, we still do - * the required flush for collapse in xfs_prepare_shift(). - * - * Insert has the same range requirements as collapse, and we extend the - * file first which can zero data. Hence insert has the same - * flush/invalidate requirements as collapse and so they are both - * handled at the right time by xfs_prepare_shift(). - */ - if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE | - FALLOC_FL_COLLAPSE_RANGE)) { - error = xfs_flush_unmap_range(ip, offset, len); - if (error) - goto out_unlock; - } - error = file_modified(file); if (error) goto out_unlock;
Call xfs_flush_unmap_range from xfs_free_file_space so that xfs_file_fallocate doesn't have to predict which mode will call it. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_bmap_util.c | 8 ++++++++ fs/xfs/xfs_file.c | 21 --------------------- 2 files changed, 8 insertions(+), 21 deletions(-)