Re: [PATCH v6 11/13] xfs: add xfs_file_dio_write_atomic()

John Garry
2025-03-18T08:42:55Z

On 18/03/2025 05:43, Christoph Hellwig wrote:
> On Mon, Mar 17, 2025 at 09:36:13AM +0000, John Garry wrote:
>>> It is only preferred if actually supported by the underlying hardware.
>>> If it isn't it really shouldn't even be tried, as that is just a waste
>>> of cycles.
>>
>> We should not even call this function if atomics are not supported by HW -
>> please see IOCB_ATOMIC checks in xfs_file_write_iter(). So maybe I will
>> mention that the caller must ensure atomics are supported for the write
>> size.
>
> I see that this is what's done in the current series now. But that feels
> very wrong. Why do you want to deprive the user of this nice and useful
> code if they don't have the right hardware?

I don't think it's fair to say that we deprive the user - so far we just
don't and nobody has asked for atomics without HW support.

> Why do we limit us to the
> hardware supported size when we support more in software?

As I see, HW offload gives fast and predictable performance.

The COW method is just a (slow) fallback is when HW offload is not possible.

If we want to allow the user to avail of atomics greater than the
mounted bdev, then we should have a method to tell the user of the
optimised threshold. They could read the bdev atomic limits and infer
this, but that is not a good user experience.

> How do you
> force test the software code if you require the hardware support?
>
>>>> + trace_xfs_file_direct_write(iocb, from);
>>>> + ret = iomap_dio_rw(iocb, from, dops, &xfs_dio_write_ops,
>>>> + dio_flags, NULL, 0);
>>>
>>> The normal direct I/O path downgrades the iolock to shared before
>>> doing the I/O here. Why isn't that done here?
>>
>> OK, I can do that. But we still require exclusive lock always for the
>> CoW-based method.
>
> If you can do away with the lock that's great and useful to get good
> performance. But if not at least document why this is different from
> others. Similarly if the COW path needs an exclusive lock document why
> in the code.

ok, I'll do that.

>
>
>>
>>>
>>>> + if (ret == -EAGAIN && !(iocb->ki_flags & IOCB_NOWAIT) &&
>>>> + dops == &xfs_direct_write_iomap_ops) {
>>>
>>> This should probably explain the unusual use of EGAIN. Although I
>>> still feel that picking a different error code for the fallback would
>>> be much more maintainable.
>>
>> I could try another error code - can you suggest one? Is it going to be
>> something unrelated to storage stack, like EREMOTEIO?
>
> Yes, the funky networking codes tends to be good candidates. E.g.
> ENOPROTOOPT for something that sounds at least vaguely related.

ok

>
>>>> +
>>>> + if (iocb->ki_flags & IOCB_ATOMIC)
>>>> + return xfs_file_dio_write_atomic(ip, iocb, from);
>>>> +
>>>
>>> Either keep space between all the conditional calls or none. I doubt
>>> just stick to the existing style.
>>
>> Sure
>
> FYI, that I doubt should have been in doubt. I was just so happy to
> finally get the mail out after a flakey connection on the train.
>

thanks