On 8/27/25 7:15 PM, Mike Snitzer wrote:
> On Wed, Aug 27, 2025 at 04:56:08PM -0400, Chuck Lever wrote:
>> On 8/27/25 3:41 PM, Mike Snitzer wrote:
>>> On Wed, Aug 27, 2025 at 11:34:03AM -0400, Chuck Lever wrote:
>>>> On 8/26/25 2:57 PM, Mike Snitzer wrote:
>>
>>>>> + if (WARN_ONCE(!nf->nf_dio_mem_align || !nf->nf_dio_read_offset_align,
>>>>> + "%s: underlying filesystem has not provided DIO alignment info\n",
>>>>> + __func__))
>>>>> + return false;
>>>>> + if (WARN_ONCE(dio_blocksize > PAGE_SIZE,
>>>>> + "%s: underlying storage's dio_blocksize=%u > PAGE_SIZE=%lu\n",
>>>>> + __func__, dio_blocksize, PAGE_SIZE))
>>>>> + return false;
>>>>
>>>> IMHO these checks do not warrant a WARN. Perhaps a trace event, instead?
>>>
>>> I won't die on this hill, I just don't see the risk of these given
>>> they are highly unlikely ("famous last words").
>>>
>>> But if they trigger we should surely be made aware immediately. Not
>>> only if someone happens to have a trace event enabled (which would
>>> only happen with further support and engineering involvement to chase
>>> "why isn't O_DIRECT being used like NFSD was optionally configured
>>> to!?").
>> A. It seems particularly inefficient to make this check for every I/O
>> rather than once per file system
>>
>> B. Once the warning has fired for one file, it won't fire again, making
>> it pretty useless if there are multiple similar mismatches. You still
>> end up with "No direct I/O even though I flipped the switch, and I
>> can't tell why."
>
> I've removed the WARN_ON_ONCEs for read and write. These repeat
> per-IO negative checks aren't ideal but they certainly aren't costly.
>
>>>>> + /* Return early if IO is irreparably misaligned (len < PAGE_SIZE,
>>>>> + * or base not aligned).
>>>>> + * Ondisk alignment is implied by the following code that expands
>>>>> + * misaligned IO to have a DIO-aligned offset and len.
>>>>> + */
>>>>> + if (unlikely(len < dio_blocksize) || ((base & (nf->nf_dio_mem_align-1)) != 0))
>>>>> + return false;
>>>>> +
>>>>> + init_nfsd_read_dio(read_dio);
>>>>> +
>>>>> + read_dio->start = round_down(offset, dio_blocksize);
>>>>> + read_dio->end = round_up(orig_end, dio_blocksize);
>>>>> + read_dio->start_extra = offset - read_dio->start;
>>>>> + read_dio->end_extra = read_dio->end - orig_end;
>>>>> +
>>>>> + /*
>>>>> + * Any misaligned READ less than NFSD_READ_DIO_MIN_KB won't be expanded
>>>>> + * to be DIO-aligned (this heuristic avoids excess work, like allocating
>>>>> + * start_extra_page, for smaller IO that can generally already perform
>>>>> + * well using buffered IO).
>>>>> + */
>>>>> + if ((read_dio->start_extra || read_dio->end_extra) &&
>>>>> + (len < NFSD_READ_DIO_MIN_KB)) {
>>>>> + init_nfsd_read_dio(read_dio);
>>>>> + return false;
>>>>> + }
>>>>> +
>>>>> + if (read_dio->start_extra) {
>>>>> + read_dio->start_extra_page = alloc_page(GFP_KERNEL);
>>>>
>>>> This introduces a page allocation where there weren't any before. For
>>>> NFSD, I/O pages come from rqstp->rq_pages[] so that memory allocation
>>>> like this is not needed on an I/O path.
>>>
>>> NFSD never supported DIO before. Yes, with this patch there is
>>> a single page allocation in the misaligned DIO READ path (if it
>>> requires reading extra before the client requested data starts).
>>>
>>> I tried to succinctly explain the need for the extra page allocation
>>> for misaligned DIO READ in this patch's header (in 2nd paragraph
>>> of the above header).
>>>
>>> I cannot see how to read extra, not requested by the client, into the
>>> head of rq_pages without causing serious problems. So that cannot be
>>> what you're saying needed.
>>>
>>>> So I think the answer to this is that I want you to implement reading
>>>> an entire aligned range from the file and then forming the NFS READ
>>>> response with only the range of bytes that the client requested, as we
>>>> discussed before.
>>>
>>> That is what I'm doing. But you're taking issue with my implementation
>>> that uses a single extra page.
>>>
>>>> The use of xdr_buf and bvec should make that quite
>>>> straightforward.
>>>
>>> Is your suggestion to, rather than allocate a disjoint single page,
>>> borrow the extra page from the end of rq_pages? Just map it into the
>>> bvec instead of my extra page?
>>
>> Yes, the extra page needs to come from rq_pages. But I don't see why it
>> should come from the /end/ of rq_pages.
>>
>> - Extend the start of the byte range back to make it align with the
>> file's DIO alignment constraint
>>
>> - Extend the end of the byte range forward to make it align with the
>> file's DIO alignment constraint
>
> nfsd_analyze_read_dio() does that (start_extra and end_extra).
>
>> - Fill in the sink buffer's bvec using pages from rq_pages, as usual
>>
>> - When the I/O is complete, adjust the offset in the first bvec entry
>> forward by setting a non-zero page offset, and adjust the returned
>> count downward to match the requested byte count from the client
>
> Tried it long ago, such bvec manipulation only works when not using
> RDMA. When the memory is remote, twiddling a local bvec isn't going
> to ensure the correct pages have the correct data upon return to the
> client.
>
> RDMA is why the pages must be used in-place, and RDMA is also why
> the extra page needed by this patch (for use as throwaway front-pad
> for expanded misaligned DIO READ) must either be allocated _or_
> hopefully it can be from rq_pages (after the end of the client
> requested READ payload).
>
> Or am I wrong and simply need to keep learning about NFSD's IO path?
You're wrong, not to put a fine point on it.
There's nothing I can think of in the RDMA or RPC/RDMA protocols that
mandates that the first page offset must always be zero. Moving data
at one address on the server to an entirely different address and
alignment on the client is exactly what RDMA is supposed to do.
It sounds like an implementation omission because the server's upper
layers have never needed it before now. If TCP already handles it, I'm
guessing it's going to be straightforward to fix.
>>> NFSD using DIO is optional. I thought the point was to get it as an
>>> available option so that _others_ could experiment and help categorize
>>> the benefits/pitfalls further?
>>
>> Yes, that is the point. But such experiments lose value if there is no
>> data collection plan to go with them.
>
> Each user runs something they care about performing well and they
> measure the result.
That assumes the user will continue to use the debug interfaces, and
the particular implementation you've proposed, for the rest of time.
And that's not my plan at all.
If we, in the community, cannot reproduce that result, or cannot
understand what has been measured, or the measurement misses part or
most of the picture, of what value is that for us to decide whether and
how to proceed with promoting the mechanism from debug feature to
something with a long-term support lifetime and a documented ABI-stable
user interface?
> Literally the same thing as has been done for anything in Linux since
> it all started. Nothing unicorn or bespoke here.
So let me ask this another way: What do we need users to measure to give
us good quality information about the page cache behavior and system
thrashing behavior you reported?
If a user asked you today "What should I measure to determine if this
mechanism is beneficial for my workload and healthy for my server" what
would you recommend?
For example: I can enable direct I/O on NFSD, but my workload is mostly
one or two clients doing kernel builds. The latency of NFS READs goes
up, but since a kernel build is not I/O bound and the client page caches
hide most of the increase, there is very little to show a measured
change.
So how should I assess and report the impact of NFSD doing direct I/O?
See -- users are not the only ones who are involved in this experiment;
and they will need guidance because we're not providing any
documentation for this feature.
>> If you would rather make this drive-by, then you'll have to realize
>> that you are requesting more than simple review from us. You'll have
>> to be content with the pace at which us overloaded maintainers can get
>> to the work.
>
> I think I just experienced the mailing-list equivalent of the Detroit
> definition of "drive-by". Good/bad news: you're a terrible shot.
The term "drive-by contribution" has a well-understood meaning in the
kernel community. If you are unfamiliar with it, I invite you to review
the mailing list archives. As always, no-one is shooting at you. If
anything, the drive-by contribution is aimed at me.
This year, Neil is not available and Jeff is working on client issues.
I will try to find some time to look at the svcrdma sendto path.
--
Chuck Lever
