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

John Garry
2023-12-13T09:32:34Z

On 12/12/2023 16:32, Christoph Hellwig wrote:
> On Tue, Dec 12, 2023 at 11:08:28AM +0000, John Garry wrote:
>> Two new fields are added to struct statx - atomic_write_unit_min and
>> atomic_write_unit_max. For each atomic individual write, the total length
>> of a write must be a between atomic_write_unit_min and
>> atomic_write_unit_max, inclusive, and a power-of-2. The write must also be
>> at a natural offset in the file wrt the write length.
>>
>> SCSI sd.c and scsi_debug and NVMe kernel support is added.
>>
>> Some open questions:
>> - How to make API extensible for when we have no HW support? In that case,
>> we would prob not have to follow rule of power-of-2 length et al.
>> As a possible solution, maybe we can say that atomic writes are
>> supported for the file via statx, but not set unit_min and max values,
>> and this means that writes need to be just FS block aligned there.
> I don't think the power of two length is much of a problem to be
> honest, and if we every want to lift it we can still do that easily
> by adding a new flag or limit.

ok, but it would be nice to have some idea on what that flag or limit
change would be.

>
> What I'm a lot more worried about is how to tell the file system that
> allocations are done right for these requirement. There is no way
> a user can know that allocations in an existing file are properly
> aligned, so atomic writes will just fail on existing files.
>
> I suspect we need an on-disk flag that forces allocations to be
> aligned to the atomic write limit, in some ways similar how the
> XFS rt flag works. You'd need to set it on an empty file, and all
> allocations after that are guaranteed to be properly aligned.

Hmmm... so how is this different to the XFS forcealign feature?

For XFS, I thought that your idea was to always CoW new extents for
misaligned extents or writes which spanned multiple extents.

>
>> - For block layer, should atomic_write_unit_max be limited by
>> max_sectors_kb? Currently it is not.
> Well. It must be limited to max_hw_sectors to actually work.

Sure, as max_hw_sectors may also be limited by DMA controller max
mapping size.

> max_sectors is a software limit below that, which with modern hardware
> is actually pretty silly and a real performance issue with todays
> workloads when people don't tweak it..

Right, so we should limit atomic write queue limits to max_hw_sectors.
But people can still tweak max_sectors, and I am inclined to say that
atomic_write_unit_max et al should be (dynamically) limited to
max_sectors also.

>
>> - How to improve requirement that iovecs are PAGE-aligned.
>> There are 2x issues:
>> a. We impose this rule to not split BIOs due to virt boundary for
>> NVMe, but there virt boundary is 4K (and not PAGE size, so broken for
>> 16K/64K pages). Easy solution is to impose requirement that iovecs
>> are 4K-aligned.
>> b. We don't enforce this rule for virt boundary == 0, i.e. SCSI
> .. we require any device that wants to support atomic writes to not
> have that silly limit. For NVMe that would require SGL support
> (and some driver changes I've been wanting to make for long where
> we always use SGLs for transfers larger than a single PRP if supported)

If we could avoid dealing with a virt boundary, then that would be nice.

Are there any patches yet for the change to always use SGLs for
transfers larger than a single PRP?

On a related topic, I am not sure about how - or if we even should -
enforce iovec PAGE-alignment or length; rather, the user could just be
advised that iovecs must be PAGE-aligned and min PAGE length to achieve
atomic_write_unit_max.

>
>
>> - Since debugging torn-writes due to unwanted kernel BIO splitting/merging
>> would be horrible, should we add some kernel storage stack software
>> integrity checks?
> Yes, I think we'll need asserts in the drivers. At least for NVMe I
> will insist on them.

Please see 16/16 in this series.

> For SCSI I think the device actually checks
> because the atomic writes are a different command anyway, or am I
> misunderstanding how SCSI works here?

Right, for WRITE ATOMIC (16) the target will reject a command when it
does not respect the device atomic write limits.

Thanks,
John