diff mbox series

[v2,13/14] fs: xfs: Validate atomic writes

Message ID 20240304130428.13026-14-john.g.garry@oracle.com (mailing list archive)
State New, archived
Headers show
Series block atomic writes for XFS | expand

Commit Message

John Garry March 4, 2024, 1:04 p.m. UTC
Validate that an atomic write adheres to length/offset rules. Since we
require extent alignment for atomic writes, this effectively also enforces
that the BIO which iomap produces is aligned.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 fs/xfs/xfs_file.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Dave Chinner March 6, 2024, 9:22 p.m. UTC | #1
On Mon, Mar 04, 2024 at 01:04:27PM +0000, John Garry wrote:
> Validate that an atomic write adheres to length/offset rules. Since we
> require extent alignment for atomic writes, this effectively also enforces
> that the BIO which iomap produces is aligned.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>  fs/xfs/xfs_file.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index d0bd9d5f596c..cecc5428fd7c 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -709,11 +709,20 @@ xfs_file_dio_write(
>  	struct kiocb		*iocb,
>  	struct iov_iter		*from)
>  {
> -	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
> +	struct inode		*inode = file_inode(iocb->ki_filp);
> +	struct xfs_inode	*ip = XFS_I(inode);
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_buftarg      *target = xfs_inode_buftarg(ip);
>  	size_t			count = iov_iter_count(from);
>  
> +	if (iocb->ki_flags & IOCB_ATOMIC) {
> +		if (!generic_atomic_write_valid(iocb->ki_pos, from,
> +			i_blocksize(inode),

a.k.a. mp->m_bsize. If you use that here, then the need for the VFS
inode goes away, too.

> +			XFS_FSB_TO_B(mp, xfs_get_extsz(ip)))) {
> +			return -EINVAL;
> +		}
> +	}

Also, I think the checks are the wrong way around here. We can only
do IOCB_ATOMIC IO on force aligned/atomic write inodes, so shouldn't
we be checking that first, then basing the rest of the checks on the
assumption that atomic writes are allowed and have been set up
correctly on the inode? i.e.

	if (iocb->ki_flags & IOCB_ATOMIC) {
		if (!xfs_inode_has_atomicwrites(ip))
			return -EINVAL;
		if (!generic_atomic_write_valid(iocb->ki_pos, from,
				mp->m_bsize, ip->i_extsize))
			return -EINVAL;
	}

because xfs_inode_has_atomicwrites() implies ip->i_extsize has been
set to the required atomic IO size?

Cheers,

Dave.
John Garry March 7, 2024, 10:19 a.m. UTC | #2
On 06/03/2024 21:22, Dave Chinner wrote:
> On Mon, Mar 04, 2024 at 01:04:27PM +0000, John Garry wrote:
>> Validate that an atomic write adheres to length/offset rules. Since we
>> require extent alignment for atomic writes, this effectively also enforces
>> that the BIO which iomap produces is aligned.
>>
>> Signed-off-by: John Garry<john.g.garry@oracle.com>
>> ---
>>   fs/xfs/xfs_file.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
>> index d0bd9d5f596c..cecc5428fd7c 100644
>> --- a/fs/xfs/xfs_file.c
>> +++ b/fs/xfs/xfs_file.c
>> @@ -709,11 +709,20 @@ xfs_file_dio_write(
>>   	struct kiocb		*iocb,
>>   	struct iov_iter		*from)
>>   {
>> -	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
>> +	struct inode		*inode = file_inode(iocb->ki_filp);
>> +	struct xfs_inode	*ip = XFS_I(inode);
>>   	struct xfs_mount	*mp = ip->i_mount;
>>   	struct xfs_buftarg      *target = xfs_inode_buftarg(ip);
>>   	size_t			count = iov_iter_count(from);
>>   
>> +	if (iocb->ki_flags & IOCB_ATOMIC) {
>> +		if (!generic_atomic_write_valid(iocb->ki_pos, from,
>> +			i_blocksize(inode),
> a.k.a. mp->m_bsize. If you use that here, then the need for the VFS
> inode goes away, too.

ok

> 
>> +			XFS_FSB_TO_B(mp, xfs_get_extsz(ip)))) {
>> +			return -EINVAL;
>> +		}
>> +	}
> Also, I think the checks are the wrong way around here. We can only
> do IOCB_ATOMIC IO on force aligned/atomic write inodes, so shouldn't
> we be checking that first,

We are checking that, but not here.

In 14/14, we only set FMODE_CAN_ATOMIC_WRITE for when 
xfs_inode_has_atomicwrites() is true, and only when 
FMODE_CAN_ATOMIC_WRITE is set can we get this far.

I don't see a point in duplicating this xfs_inode_has_atomicwrites() 
check, so I will make the commit message clearer on this - ok? Or add a 
comment.

> then basing the rest of the checks on the
> assumption that atomic writes are allowed and have been set up
> correctly on the inode? i.e.
> 
> 	if (iocb->ki_flags & IOCB_ATOMIC) {
> 		if (!xfs_inode_has_atomicwrites(ip))
> 			return -EINVAL;
> 		if (!generic_atomic_write_valid(iocb->ki_pos, from,
> 				mp->m_bsize, ip->i_extsize))
> 			return -EINVAL;
> 	}
> 
> because xfs_inode_has_atomicwrites() implies ip->i_extsize has been
> set to the required atomic IO size?

I was not too comfortable using ip->i_extsize, as this can be set 
without forcealign being set. I know that we would not get this far 
without forcealign (being set).

Having said that, I don't like all the xfs_get_extsz() calls, so 
something better is required. Let me know you you think.

Thanks,
John
diff mbox series

Patch

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index d0bd9d5f596c..cecc5428fd7c 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -709,11 +709,20 @@  xfs_file_dio_write(
 	struct kiocb		*iocb,
 	struct iov_iter		*from)
 {
-	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
+	struct inode		*inode = file_inode(iocb->ki_filp);
+	struct xfs_inode	*ip = XFS_I(inode);
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_buftarg      *target = xfs_inode_buftarg(ip);
 	size_t			count = iov_iter_count(from);
 
+	if (iocb->ki_flags & IOCB_ATOMIC) {
+		if (!generic_atomic_write_valid(iocb->ki_pos, from,
+			i_blocksize(inode),
+			XFS_FSB_TO_B(mp, xfs_get_extsz(ip)))) {
+			return -EINVAL;
+		}
+	}
+
 	/* direct I/O must be aligned to device logical sector size */
 	if ((iocb->ki_pos | count) & target->bt_logical_sectormask)
 		return -EINVAL;