Message-ID: <aaa33b4f-dea7-4596-82ce-8c7e6cdaa6ef@oracle.com>
Date: Wed, 10 Jan 2024 08:55:06 +0000
From: John Garry <john.g.garry@...cle.com>
To: Dave Chinner <david@...morbit.com>
Cc: Christoph Hellwig <hch@....de>, "Darrick J. Wong" <djwong@...nel.org>,
        axboe@...nel.dk, kbusch@...nel.org, sagi@...mberg.me,
        jejb@...ux.ibm.com, martin.petersen@...cle.com,
        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-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        tytso@....edu, jbongio@...gle.com, linux-scsi@...r.kernel.org,
        ming.lei@...hat.com, bvanassche@....org, ojaswin@...ux.ibm.com
Subject: Re: [PATCH v2 00/16] block atomic writes

On 09/01/2024 23:04, Dave Chinner wrote:
>> --- a/include/uapi/linux/fs.h
>> +++ b/include/uapi/linux/fs.h
>> @@ -118,7 +118,8 @@ struct fsxattr {
>>         __u32           fsx_nextents;   /* nextents field value (get)   */
>>         __u32           fsx_projid;     /* project identifier (get/set) */
>>         __u32           fsx_cowextsize; /* CoW extsize field value
>> (get/set)*/
>> -       unsigned char   fsx_pad[8];
>> +       __u32           fsx_atomicwrites_size; /* unit max */
>> +       unsigned char   fsx_pad[4];
>> };
>>
>> /*
>> @@ -140,6 +141,7 @@ struct fsxattr {
>> #define FS_XFLAG_FILESTREAM    0x00004000      /* use filestream allocator
>> */
>> #define FS_XFLAG_DAX           0x00008000      /* use DAX for IO */
>> #define FS_XFLAG_COWEXTSIZE    0x00010000      /* CoW extent size
>> allocator hint */
>> +#define FS_XFLAG_ATOMICWRITES  0x00020000
>> #define FS_XFLAG_HASATTR       0x80000000      /* no DIFLAG for this   */
>>
>> /* the read-only stuff doesn't really belong here, but any other place is
>> lines 1-22/22 (END)
>>
>> Having FS_XFLAG_ATOMICWRITES set will lead to FMODE_CAN_ATOMIC_WRITE being
>> set.
>>
>> So a user can issue:
>>
>>> xfs_io -c "atomic-writes 64K" mnt/file
>>> xfs_io -c "atomic-writes" mnt/file
>> [65536] mnt/file
> Where are you going to store this value in the inode?  It requires a
> new field in the inode and so is a change of on-disk format, right?

It would require an on-disk format change, unless we can find an 
alternative way to store the value, like:
a. re-use pre-existing extsize or even cowextsize fields and 'xfs_io -c 
"atomic-writes $SIZE"' would update those fields and 
FS_XFLAG_ATOMICWRITES would be incompatible with FS_XFLAG_COWEXTSIZE or 
FS_XFLAG_EXTSIZE
b. require FS_XFLAG_EXTSIZE and extsize be also set to enable atomic 
writes, and extsize is used for atomic write unit max

I'm trying to think of ways to avoid requiring a value, but I don't see 
good options, like:
- make atomic write unit max some compile-time option
- require mkfs stripe alignment/width be set and use that as basis for 
atomic write unit max

We could just use the atomic write unit max which HW provides, but that 
could be 1MB or more and that will hardly give efficient data usage for 
small files. But maybe we don't care about that if we expect this 
feature to only be used on DB files, which can be huge anyway. However I 
still have concerns – we require that value to be fixed, but a disk 
firmware update could increase that value and this could mean we have 
what would be pre-existing mis-aligned extents.

> 
> As it is, I really don't see this as a better solution than the
> original generic "force align" flag that simply makes the extent
> size hint alignment a hard physical alignment requirement rather
> than just a hint. This has multiple uses (DAX PMD alignment is
> another), so I just don't see why something that has a single,
> application specific API that implements a hard physical alignment
> is desirable.

I would still hope that we will support forcealign separately for those 
purposes.

> 
> Indeed, the whole reason that extent size hints are so versatile is
> that they implement a generic allocation alignment/size function
> that can be used for anything your imagination extends to. If they
> were implemented as a "only allow RAID stripe aligned/sized
> allocation" for the original use case then that functionality would
> have been far less useful than it has proven to be over the past
> couple of decades.
> 
> Hence history teaches us that we should be designing the API around
> the generic filesystem function required (hard alignment of physical
> extent allocation), not the specific use case that requires that
> functionality.

I understand your concern. However I am not even sure that forcealign 
even gives us everything we want to enable atomic writes. There is an 
issue where we were required to pre-zero a file prior to issuing atomic 
writes to ensure extents are suitably sized, so FS_XFLAG_ATOMICWRITES 
would make the FS do what is required to avoid that pre-zeroing (but 
that pre-zeroing requirement that does sound like a forcealign issue...)

Furthermore, there was some desire to support atomic writes on block 
devices with no HW support by using a CoW-based solution, and forcealign 
would not be relevant there.

Thanks,
John

Powered by blists - more mailing lists