[PATCH] xfs: Check for deallayed allocations before setting extsize

[PATCH] xfs: Check for deallayed allocations before setting extsize

[Ojaswin Mujoo]

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(-)

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;
 
-- 
2.43.5

Re: [PATCH] xfs: Check for deallayed allocations before setting extsize

[Christoph Hellwig]

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>

Re: [PATCH] xfs: Check for deallayed allocations before setting extsize

[Ojaswin Mujoo]

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

Re: [PATCH] xfs: Check for deallayed allocations before setting extsize

[Christoph Hellwig]

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.