| Message ID | 20241003101207.205083-1-ojaswin@linux.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | xfs: Check for deallayed allocations before setting extsize | expand |
On Thu, Oct 03, 2024 at 03:42:07PM +0530, Ojaswin Mujoo wrote: > Extsize is allowed to be set on files with no data in it. For this, > we were checking if the files have extents but missed to check if > delayed extents were present. This patch adds that check. > > **Without the patch (SUCCEEDS)** > > $ xfs_io -c 'open -f testfile' -c 'pwrite 0 1024' -c 'extsize 65536' Can you add a testcase for this to xfstests? > - if (S_ISREG(VFS_I(ip)->i_mode) && ip->i_df.if_nextents && > + if (S_ISREG(VFS_I(ip)->i_mode) && > + (ip->i_df.if_nextents || ip->i_delayed_blks) && We have two other copies of the ip->i_df.if_nextents || ip->i_delayed_blks pattern to check if there is any data on the inode in xfs_inactive and xfs_ioctl_setattr_xflags. Maybe facto this into a documented helper? Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, Oct 03, 2024 at 07:38:11AM -0700, Christoph Hellwig wrote: > On Thu, Oct 03, 2024 at 03:42:07PM +0530, Ojaswin Mujoo wrote: > > Extsize is allowed to be set on files with no data in it. For this, > > we were checking if the files have extents but missed to check if > > delayed extents were present. This patch adds that check. > > > > **Without the patch (SUCCEEDS)** > > > > $ xfs_io -c 'open -f testfile' -c 'pwrite 0 1024' -c 'extsize 65536' > > Can you add a testcase for this to xfstests? Hi Christoph, actually now that we are also planning to use this for atomic writes, we are thinking to add a generic extsize ioctl test to check for: 1. Setting hint on empty file should pass 2. Setting hint on a file with delayed allocation data should fail 3. Setting hint on a file with allocated data should fail 4. Setting hint on a file which is truncated to size 0 after write should pass So that should cover this for ext4 and xfs as well. > > > - if (S_ISREG(VFS_I(ip)->i_mode) && ip->i_df.if_nextents && > > + if (S_ISREG(VFS_I(ip)->i_mode) && > > + (ip->i_df.if_nextents || ip->i_delayed_blks) && > > We have two other copies of the > > ip->i_df.if_nextents || ip->i_delayed_blks > > pattern to check if there is any data on the inode in xfs_inactive and > xfs_ioctl_setattr_xflags. Maybe facto this into a documented helper? Sure I can do this. > > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Thanks for the review. Regards, ojaswin
On Fri, Oct 04, 2024 at 02:26:07PM +0530, Ojaswin Mujoo wrote: > Hi Christoph, actually now that we are also planning to use this for > atomic writes, we are thinking to add a generic extsize ioctl > test to check for: > > 1. Setting hint on empty file should pass > 2. Setting hint on a file with delayed allocation data should fail > 3. Setting hint on a file with allocated data should fail > 4. Setting hint on a file which is truncated to size 0 after write should pass > > So that should cover this for ext4 and xfs as well. Sounds good.
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 7226d27e8afc..55b574b43617 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -602,7 +602,8 @@ xfs_ioctl_setattr_check_extsize( if (!fa->fsx_valid) return 0; - if (S_ISREG(VFS_I(ip)->i_mode) && ip->i_df.if_nextents && + if (S_ISREG(VFS_I(ip)->i_mode) && + (ip->i_df.if_nextents || ip->i_delayed_blks) && XFS_FSB_TO_B(mp, ip->i_extsize) != fa->fsx_extsize) return -EINVAL;
Extsize is allowed to be set on files with no data in it. For this, we were checking if the files have extents but missed to check if delayed extents were present. This patch adds that check. **Without the patch (SUCCEEDS)** $ xfs_io -c 'open -f testfile' -c 'pwrite 0 1024' -c 'extsize 65536' wrote 1024/1024 bytes at offset 0 1 KiB, 1 ops; 0.0002 sec (4.628 MiB/sec and 4739.3365 ops/sec) **With the patch (FAILS as expected)** $ xfs_io -c 'open -f testfile' -c 'pwrite 0 1024' -c 'extsize 65536' wrote 1024/1024 bytes at offset 0 1 KiB, 1 ops; 0.0002 sec (4.628 MiB/sec and 4739.3365 ops/sec) xfs_io: FS_IOC_FSSETXATTR testfile: Invalid argument Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> --- fs/xfs/xfs_ioctl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)