Re: [PATCH v2 00/16] block atomic writes

John Garry
2024-01-10T08:55:46Z

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