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

[Christoph Hellwig <hch@lst.de>]

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?  Why do we limit us to the
hardware supported size when we support more in software?  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.


>
>>
>>> +	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.

>>> +
>>> +	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.