linux-kernel - Re: [PATCH v7 2/9] fs: Initial 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: <fbb835ff-f1ae-4b59-8cb3-22a11449d781@oracle.com>
Date: Wed, 5 Jun 2024 11:48:12 +0100
From: John Garry <john.g.garry@...cle.com>
To: Christoph Hellwig <hch@....de>
Cc: axboe@...nel.dk, kbusch@...nel.org, sagi@...mberg.me, jejb@...ux.ibm.com,
        martin.petersen@...cle.com, djwong@...nel.org, viro@...iv.linux.org.uk,
        brauner@...nel.org, dchinner@...hat.com, jack@...e.cz,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-nvme@...ts.infradead.org, linux-fsdevel@...r.kernel.org,
        tytso@....edu, jbongio@...gle.com, linux-scsi@...r.kernel.org,
        ojaswin@...ux.ibm.com, linux-aio@...ck.org,
        linux-btrfs@...r.kernel.org, io-uring@...r.kernel.org,
        nilay@...ux.ibm.com, ritesh.list@...il.com, willy@...radead.org,
        Prasad Singamsetty <prasad.singamsetty@...cle.com>
Subject: Re: [PATCH v7 2/9] fs: Initial atomic write support

On 05/06/2024 09:30, Christoph Hellwig wrote:
> Highlevel question:  in a lot of the discussions we've used the
> term "untorn writes" instead, which feels better than atomic to
> me as atomic is a highly overloaded term.  Should we switch the
> naming to that?

I have no strong attachment to that name (atomic).

For both SCSI and NVMe, it's an "atomic" feature and I was basing the 
naming on that.

We could have RWF_NOTEARS or RWF_UNTEARABLE_WRITE or RWF_UNTEARABLE or 
RWF_UNTORN or similar. Any preference?

> 
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 0283cf366c2a..6cb67882bcfd 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -45,6 +45,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/maple_tree.h>
>>   #include <linux/rw_hint.h>
>> +#include <linux/uio.h>
> 
> fs.h is included almost everywhere, so if we can avoid pulling in
> even more dependencies that would be great.
> 
> It seems like it is pulled in just for this helper:

right

> 
>> +static inline
>> +bool generic_atomic_write_valid(loff_t pos, struct iov_iter *iter)
>> +{
>> +	size_t len = iov_iter_count(iter);
>> +
>> +	if (!iter_is_ubuf(iter))
>> +		return false;
>> +
>> +	if (!is_power_of_2(len))
>> +		return false;
>> +
>> +	if (!IS_ALIGNED(pos, len))
>> +		return false;
>> +
>> +	return true;
>> +}
> 
> should that just go to uio.h instead, or move out of line?

ok, I am not sure about moving to uio.h, but I'll try to do something 
about this issue

> 
> Also the return type formatting is wrong, the two normal styles are
> either:
> 
> static inline bool generic_atomic_write_valid(loff_t pos,
> 		struct iov_iter *iter)
> 
> or:
> 
> static inline bool
> generic_atomic_write_valid(loff_t pos, struct iov_iter *iter)
> 
> (and while I'm at nitpicking, passing the pos before the iter
> feels weird)

generally pos is first and then len (which iter provides) when a 
function accepts position and length, but then iter is the "larger" arg, 
and normally they go first. Anyway I don't mind changing that as you 
suggest.

> 
> Last but not least: if READ/WRITE is passed to kiocb_set_rw_flags,
> it should probably set IOCB_WRITE as well?  That might be a worthwile
> prep patch on it's own.

For io_uring/rw.c, we have io_write() -> io_rw_init_file(..., WRITE), 
and then later we set IOCB_WRITE, so would be neat to use there. But 
then do_iter_readv_writev() does not set IOCB_WRITE - I can't imagine 
that setting IOCB_WRITE would do any harm there. I see a similar change 
in 
https://lore.kernel.org/linux-fsdevel/167391048988.2311931.1567396746365286847.stgit@warthog.procyon.org.uk/

AFAICS, setting IOCB_WRITE is quite inconsistent. From browsing through 
fsdevel on lore, there was some history in trying to use IOCB_WRITE 
always instead of iov_iter direction. Any idea what happened to that?

I'm just getting the feeling that setting IOCB_WRITE in 
kiocb_set_rw_flags() is a small part - and maybe counter productive - of 
a larger job of fixing IOCB_WRITE usage.

Thanks,
John