linux-kernel - Re: [PATCH v10 5/8] fs: iomap: Atomic write support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87o73fgg3e.fsf@gmail.com>
Date: Sun, 20 Oct 2024 17:07:41 +0530
From: Ritesh Harjani (IBM) <ritesh.list@...il.com>
To: John Garry <john.g.garry@...cle.com>, axboe@...nel.dk, brauner@...nel.org, djwong@...nel.org, viro@...iv.linux.org.uk, jack@...e.cz, dchinner@...hat.com, hch@....de, cem@...nel.org
Cc: linux-block@...r.kernel.org, linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org, hare@...e.de, martin.petersen@...cle.com, catherine.hoang@...cle.com, mcgrof@...nel.org, ojaswin@...ux.ibm.com
Subject: Re: [PATCH v10 5/8] fs: iomap: Atomic write support

John Garry <john.g.garry@...cle.com> writes:

> On 20/10/2024 09:21, Ritesh Harjani (IBM) wrote:
>>>   -293,7 +295,8 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>>>   	const struct iomap *iomap = &iter->iomap;
>>>   	struct inode *inode = iter->inode;
>>>   	unsigned int fs_block_size = i_blocksize(inode), pad;
>>> -	loff_t length = iomap_length(iter);
>>> +	const loff_t length = iomap_length(iter);
>>> +	bool atomic = iter->flags & IOMAP_ATOMIC;
>>>   	loff_t pos = iter->pos;
>>>   	blk_opf_t bio_opf;
>>>   	struct bio *bio;
>>> @@ -303,6 +306,9 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>>>   	size_t copied = 0;
>>>   	size_t orig_count;
>>>   
>>> +	if (atomic && length != fs_block_size)
>>> +		return -EINVAL;
>> We anyway mandate iov_iter_count() write should be same as sb_blocksize
>> in xfs_file_write_iter() for atomic writes.
>> This comparison here is not required. I believe we do plan to lift this
>> restriction maybe when we are going to add forcealign support right?
>
> Yes, we would lift this restriction if and when forcealign is added. Or 
> when bigalloc is leveraged for ext4 atomic writes.
>
> But I think that today it is proper to add this check, as we are saying 
> that iomap DIO path does not support anything else than fs_block_size.
>
> For forcealign, we were introducing support for atomic writes spanning 
> mixed unwritten and written extents in [0]. We don't have that support 
> here, so it is prudent to say that we just support fs_block_size.
>
> [0] 
> https://lore.kernel.org/linux-xfs/20240607143919.2622319-4-john.g.garry@oracle.com/
>

Sure.

>> 
>> And similarly this needs to be lifted when ext4 adds support for atomic
>> write even with bigalloc. I hope we can do so when we add such support, right?
>
> Right
>

Thanks for confirming that.
The patch looks good to me. Please feel free to add - 

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>