NFSD: Implement NFSD_IO_DIRECT for NFS WRITE

The client specifies with the stable parameter the method of how the data is to be processed by the server. If stable is FILE_SYNC4, the server MUST commit the data written plus all file system metadata to stable storage before returning results. This corresponds to the NFSv2 protocol semantics. Any other behavior constitutes a protocol violation. If stable is DATA_SYNC4, then the server MUST commit all of the data to stable storage and enough of the metadata to retrieve the data before returning.

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1345

Quoted 7 lines · Chuck Lever

fs/nfsd/vfs.c +7 -0

+no_dio:
+	/*
+	 * No DIO alignment possible - pack into single non-DIO segment.
+	 * IOCB_DONTCACHE preserves the intent of NFSD_IO_DIRECT.
+	 */
+	nfsd_write_dio_seg_init(&segments[0], bvec, nvecs, total, 0,
+				total, iocb);

I'd like to sort out the discussion on why to set IOCB_DONTCACHE when nothing is aligned, but not for the non-aligned parts as that is extremely counter-intuitive. From the other thread it might be because the test case used to justify it is very unaligned and caching partial pages is helpful, but if that is indeed the case the right check would be for writes that are file offset unaligned vs the page or max folio size and not about being able to do parts of it as direct I/O.

Either way a tweak to suddenly use cached buffered I/O when the mode asks for direct should have a comment explaining the justification and explain the rationale instead of rushing it in.

Quoted 16 lines · Christoph Hellwig

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1345

Quoted 7 lines · Chuck Lever

fs/nfsd/vfs.c +7 -0

+no_dio:
+	/*
+	 * No DIO alignment possible - pack into single non-DIO segment.
+	 * IOCB_DONTCACHE preserves the intent of NFSD_IO_DIRECT.
+	 */
+	nfsd_write_dio_seg_init(&segments[0], bvec, nvecs, total, 0,
+				total, iocb);

I'd like to sort out the discussion on why to set IOCB_DONTCACHE when nothing is aligned, but not for the non-aligned parts as that is extremely counter-intuitive. From the other thread it might be because the test case used to justify it is very unaligned and caching partial pages is helpful, but if that is indeed the case the right check would be for writes that are file offset unaligned vs the page or max folio size and not about being able to do parts of it as direct I/O.

Quoted 3 lines

Either way a tweak to suddenly use cached buffered I/O when the mode asks for direct should have a comment explaining the justification and explain the rationale instead of rushing it in.

Agreed. The cover letter noted that this is still controversial.

Quoted 1 line

Agreed. The cover letter noted that this is still controversial.

Only see this, must be missing what you're referring to:

Changes since v9:
* Unaligned segments no longer use IOCB_DONTCACHE

One controversy remains: Whether to set DONTCACHE for the unaligned segments.

Quoted 5 lines · Mike Snitzer

Quoted 1 line

Agreed. The cover letter noted that this is still controversial.

Only see this, must be missing what you're referring to:

Changes since v9:
* Unaligned segments no longer use IOCB_DONTCACHE

From the v11 cover letter:

Quoted 2 lines

One controversy remains: Whether to set DONTCACHE for the unaligned segments.

Quoted 7 lines · Chuck Lever

Quoted 5 lines · Mike Snitzer

Quoted 1 line

Agreed. The cover letter noted that this is still controversial.

Only see this, must be missing what you're referring to:

Changes since v9:
* Unaligned segments no longer use IOCB_DONTCACHE

Quoted 4 lines

From the v11 cover letter:

Quoted 2 lines

One controversy remains: Whether to set DONTCACHE for the unaligned segments.

Ha, blind as a bat...

hopefully the rest of my reply helps dispel the controversy

Quoted 8 lines · Mike Snitzer

Quoted 7 lines · Chuck Lever

Quoted 5 lines · Mike Snitzer

Quoted 1 line

Agreed. The cover letter noted that this is still controversial.

Only see this, must be missing what you're referring to:

Changes since v9:
* Unaligned segments no longer use IOCB_DONTCACHE

Quoted 9 lines

Quoted 4 lines

From the v11 cover letter:

Quoted 2 lines

One controversy remains: Whether to set DONTCACHE for the unaligned segments.

Ha, blind as a bat...

hopefully the rest of my reply helps dispel the controversy

Unfortunately I don't think it does. I don't think it even addresses it.

What Christoph said was:

I'd like to sort out the discussion on why to set IOCB_DONTCACHE when nothing is aligned, but not for the non-aligned parts as that is extremely counter-intuitive.

You gave a lengthy (and probably valid) description on why "not for the non-aligned parts" but don't seem to address the "why to set IOCB_DONTCACHE when nothing is aligned" bit.

I can see Christoph's point.

Can you please explain why all the arguments you have for avoiding IOCB_DONTCACHE on the non-aligned head and tail do not equally apply when the whole body is assessed as non-aligned. i.e. the no_dio label in nfsd_write_dio_iters_init().

Quoted 28 lines · NeilBrown

Quoted 8 lines · Mike Snitzer

Quoted 7 lines · Chuck Lever

Quoted 5 lines · Mike Snitzer

Quoted 1 line

Agreed. The cover letter noted that this is still controversial.

Only see this, must be missing what you're referring to:

Changes since v9:
* Unaligned segments no longer use IOCB_DONTCACHE

Quoted 9 lines

Quoted 4 lines

From the v11 cover letter:

Quoted 2 lines

One controversy remains: Whether to set DONTCACHE for the unaligned segments.

Ha, blind as a bat...

hopefully the rest of my reply helps dispel the controversy

Unfortunately I don't think it does. I don't think it even addresses it.

What Christoph said was:

I'd like to sort out the discussion on why to set IOCB_DONTCACHE when nothing is aligned, but not for the non-aligned parts as that is extremely counter-intuitive.

Quoted 3 lines

You gave a lengthy (and probably valid) description on why "not for the non-aligned parts" but don't seem to address the "why to set IOCB_DONTCACHE when nothing is aligned" bit.

Because if the entire IO is so poorly formed that it cannot be issued with DIO then at least we can provide the spirit of what was requested from the admin having cared to enable NFSD_IO_DIRECT.

Those IOs could be quite large.. full WRITE payload. I have a reproducer if you'd like it!

Limiting extensive use of caching buffered IO is pretty important if the admin enabled NFSD_IO_DIRECT.

A misaligned WRITE's non-aligned head and tail are _not_ extensive amounts of mmeory. They are at most 2 individial pages.

Quoted 6 lines

I can see Christoph's point.

Can you please explain why all the arguments you have for avoiding IOCB_DONTCACHE on the non-aligned head and tail do not equally apply when the whole body is assessed as non-aligned. i.e. the no_dio label in nfsd_write_dio_iters_init().

Sure, hopefully I just did above.

Quoted 33 lines · Mike Snitzer

Quoted 31 lines · NeilBrown

Quoted 8 lines · Mike Snitzer

Quoted 7 lines · Chuck Lever

Quoted 5 lines · Mike Snitzer

Quoted 1 line

Agreed. The cover letter noted that this is still controversial.

Only see this, must be missing what you're referring to:

Changes since v9:
* Unaligned segments no longer use IOCB_DONTCACHE

Quoted 9 lines

Quoted 4 lines

From the v11 cover letter:

Quoted 2 lines

One controversy remains: Whether to set DONTCACHE for the unaligned segments.

Ha, blind as a bat...

hopefully the rest of my reply helps dispel the controversy

Unfortunately I don't think it does. I don't think it even addresses it.

What Christoph said was:

I'd like to sort out the discussion on why to set IOCB_DONTCACHE when nothing is aligned, but not for the non-aligned parts as that is extremely counter-intuitive.

You gave a lengthy (and probably valid) description on why "not for the non-aligned parts" but don't seem to address the "why to set IOCB_DONTCACHE when nothing is aligned" bit.

Quoted 3 lines

Because if the entire IO is so poorly formed that it cannot be issued with DIO then at least we can provide the spirit of what was requested from the admin having cared to enable NFSD_IO_DIRECT.

"so poorly formed"... How can IO be poorly formed in a way that is worse than not being aligned?

Quoted 2 lines

Those IOs could be quite large.. full WRITE payload. I have a reproducer if you'd like it!

Can they? How does that work?
From looking at the code I can only see that "Those IOs" are too small to have any whole pages in them.

Maybe we need a clear description of the sort of IOs that are big enough that they impose a performance cost on the page-cache, but that somehow cannot be usefully split into a prefix/middle/suffix where prefix and suffix are less than a page each.

Q: Case 2 uses DONTCACHE, so case 1 should too right?

A: NO. There is legit benefit to having case 1 use cached buffered IO
when issuing its 2 subpage IOs; and that benefit doesn't cause harm> because order-0 page management is not causing the MM problems that NFSD_IO_DIRECT sets out to avoid (whereas higher order cached buffered
IO is exactly what both DONTCACHE and NFSD_IO_DIRECT aim to avoid.
Otherwise MM spins and spins trying to find adequate free pages,
cannot so does dirty writeback and reclaim which causes kswapd and
kcompactd to burn cpu, etc).

Let's please not get hung up of intent of O_DIRECT because NFSD_IO_DIRECT achieves that intent very well

Quoted 9 lines · Mike Snitzer

Q: Case 2 uses DONTCACHE, so case 1 should too right?

A: NO. There is legit benefit to having case 1 use cached buffered IO
when issuing its 2 subpage IOs; and that benefit doesn't cause harm> because order-0 page management is not causing the MM problems that NFSD_IO_DIRECT sets out to avoid (whereas higher order cached buffered
IO is exactly what both DONTCACHE and NFSD_IO_DIRECT aim to avoid.
Otherwise MM spins and spins trying to find adequate free pages,
cannot so does dirty writeback and reclaim which causes kswapd and
kcompactd to burn cpu, etc).

Paraphrasing: Each unaligned end (case 1) is always smaller than a page, thus it will stick with order-0 allocations (if that byte range is not already in the page cache), allocations that are, practically speaking, reliable.

However it might be a stretch to claim that an order-0 allocation /never/ drives memory reclaim.

It still comes down to "it's faster and generally not harmful... and clients have to issue WRITEs that are arbitrarily aligned, so let's help them out."

What we still don't know is exactly what the extra cost of setting DONTCACHE is, even on small writes. Maybe DONTCACHE should be cleared for /all/ segments that are smaller than a page?

Sidebar: I resist calling such writes poorly formed or misaligned, as there seems to be a little (unintended) moralism in those terms. Clients must write only what data has changed. Aligning the payload byte ranges (using RMW) is incredibly inefficient. So those writes are just as correct and valid as writes that are optimally aligned.

When I hear "poorly formed" write, I think of an NFS WRITE that has invalid arguments or corrupted XDR.

Quoted 2 lines

Let's please not get hung up of intent of O_DIRECT because NFSD_IO_DIRECT achieves that intent very well

I think we need to have a clear idea what that intent is, because it is explicitly referenced in a code comment as the rationale for setting DONTCACHE.

It might be better to replace that comment with a reason for using DONTCACHE that does not mention "the intent of using DIRECT". Something like:

* Mark the I/O buffer as evict-able to reduce memory contention.

Quoted 9 lines · Mike Snitzer

Q: Case 2 uses DONTCACHE, so case 1 should too right?

A: NO. There is legit benefit to having case 1 use cached buffered IO
when issuing its 2 subpage IOs; and that benefit doesn't cause harm> because order-0 page management is not causing the MM problems that NFSD_IO_DIRECT sets out to avoid (whereas higher order cached buffered
IO is exactly what both DONTCACHE and NFSD_IO_DIRECT aim to avoid.
Otherwise MM spins and spins trying to find adequate free pages,
cannot so does dirty writeback and reclaim which causes kswapd and
kcompactd to burn cpu, etc).

Paraphrasing: Each unaligned end (case 1) is always smaller than a page, thus it will stick with order-0 allocations (if that byte range is not already in the page cache), allocations that are, practically speaking, reliable.

However it might be a stretch to claim that an order-0 allocation /never/ drives memory reclaim.

What we still don't know is exactly what the extra cost of setting DONTCACHE is, even on small writes. Maybe DONTCACHE should be cleared for /all/ segments that are smaller than a page?

Sidebar: I resist calling such writes poorly formed or misaligned, as there seems to be a little (unintended) moralism in those terms. Clients must write only what data has changed. Aligning the payload byte ranges (using RMW) is incredibly inefficient. So those writes are just as correct and valid as writes that are optimally aligned.

When I hear "poorly formed" write, I think of an NFS WRITE that has invalid arguments or corrupted XDR.

Quoted 3 lines

What we still don't know is exactly what the extra cost of setting DONTCACHE is, even on small writes. Maybe DONTCACHE should be cleared for /all/ segments that are smaller than a page?

I suspect the best initial tweak is for every segment or entire write that is not page aligned in the file, as that is an indicator that multiple RMW cycles are possible. At least if we're streaming, but we don't have that information. That means all of these cases:

1) writes smaller than PAGE_SIZE
2) writes smaller than PAGE_SIZE * 2 but not aligned to PAGE_SIZE
3) unaligned end segments < PAGE_SIZE

If we want to fine tune, we'd probably expand case 2 a bit as a single page cache operation on an order 2 pages is going to be faster than three I/Os most of the time, but compared to the high level discussion here that's minor details.

To move forward before it is too late to hit v6.19, what I'm thinking is this:

* I'll reset the patch to leave DONTCACHE set for all unaligned segments, as it was in the original logic

* I'll post a final version of the series to be merged

* We can resume this discussion after the series is merged, as an opportunity for optimization

Quoted 16 lines · Chuck Lever

Quoted 9 lines · Mike Snitzer

Q: Case 2 uses DONTCACHE, so case 1 should too right?

A: NO. There is legit benefit to having case 1 use cached buffered IO
when issuing its 2 subpage IOs; and that benefit doesn't cause harm> because order-0 page management is not causing the MM problems that NFSD_IO_DIRECT sets out to avoid (whereas higher order cached buffered
IO is exactly what both DONTCACHE and NFSD_IO_DIRECT aim to avoid.
Otherwise MM spins and spins trying to find adequate free pages,
cannot so does dirty writeback and reclaim which causes kswapd and
kcompactd to burn cpu, etc).

Paraphrasing: Each unaligned end (case 1) is always smaller than a page, thus it will stick with order-0 allocations (if that byte range is not already in the page cache), allocations that are, practically speaking, reliable.

Quoted 2 lines

However it might be a stretch to claim that an order-0 allocation /never/ drives memory reclaim.

That is of course not true. order-0 pages of course do drive memory reclaim. In fact if you're on a file system without huge folios and an applications that doesn't use THP, the majority of reclaim is driven by order 0 allocations.

Quoted 3 lines

What we still don't know is exactly what the extra cost of setting DONTCACHE is, even on small writes. Maybe DONTCACHE should be cleared for /all/ segments that are smaller than a page?

I suspect the best initial tweak is for every segment or entire write that is not page aligned in the file, as that is an indicator that multiple RMW cycles are possible. At least if we're streaming, but we don't have that information. That means all of these cases:

1) writes smaller than PAGE_SIZE
2) writes smaller than PAGE_SIZE * 2 but not aligned to PAGE_SIZE
3) unaligned end segments < PAGE_SIZE

If we want to fine tune, we'd probably expand case 2 a bit as a single page cache operation on an order 2 pages is going to be faster than three I/Os most of the time, but compared to the high level discussion here that's minor details.

Review

Quoted 4 lines · Mike Snitzer

(quoted diff) +4 -0

+	/* Mark the I/O buffer as evict-able to reduce memory contention. */
+	if (!use_cached_buffered_fallback &&
+	    nf->nf_file->f_op->fop_flags & FOP_DONTCACHE)
+		kiocb->ki_flags |= IOCB_DONTCACHE;

This won't work, because ki_flags get overwritten by the flags in the segment layer.

I walked through this to understand all the cases,
and ended up with a slightly different version including long comments explaining them. This is also untested, but I'd love to have other cross check the logic in it:

fs/nfsd/vfs.c +0 -0

index ab46301da4ae..4727235ef43f 100644
@@ -1292,12 +1292,23 @@ nfsd_write_dio_iters_init(struct nfsd_file *nf, struct bio_vec *bvec,
 	unsigned int nsegs = 0;

fs/nfsd/vfs.c +5 -1

+	if (unlikely(total < max(offset_align, mem_align)))
 		goto no_dio;
 
 	prefix_end = round_up(offset, offset_align);

@@ -1308,7 +1319,17 @@ nfsd_write_dio_iters_init(struct nfsd_file *nf, struct bio_vec *bvec,
 	middle = middle_end - prefix_end;
 	suffix = orig_end - middle_end;
 
-	if (!middle)
+	/*
+	 * If there is no aligned middle section, or aligned part is tiny,
+	 * issue a single buffered I/O write instead of splitting up the
+	 * write.

fs/nfsd/vfs.c +3 -0

@@ -1324,16 +1345,24 @@ nfsd_write_dio_iters_init(struct nfsd_file *nf, struct bio_vec *bvec,
 	 * bvecs generated from RPC receive buffers are contiguous: After
 	 * the first bvec, all subsequent bvecs start at bv_offset zero
 	 * (page-aligned). Therefore, only the first bvec is checked.
+	 *
+	 * If the memory is not aligned at all, but we have a large enough
+	 * logical offset-aligned middle section, try to use uncached buffered

fs/nfsd/vfs.c +4 -2

+	} else {
+		segments[nsegs].flags |= IOCB_DIRECT;
+	}
 	nsegs++;
 
 	if (suffix)
 		nfsd_write_dio_seg_init(&segments[nsegs++], bvec, nvecs, total,
 					prefix + middle, suffix, iocb);
-
 	return nsegs;
 
 no_dio:

@@ -1362,16 +1391,9 @@ nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		if (kiocb->ki_flags & IOCB_DIRECT)
 			trace_nfsd_write_direct(rqstp, fhp, kiocb->ki_pos,
 						segments[i].iter.count);
-		else {
+		else
 			trace_nfsd_write_vector(rqstp, fhp, kiocb->ki_pos,
 						segments[i].iter.count);

fs/nfsd/vfs.c nfsd_write_dio_iters_init(struct nfsd_file *nf, struct bio_vec *bvec, · line 1299

Quoted 8 lines

fs/nfsd/vfs.c +4 -3

-	 * Check if direct I/O is feasible for this write request.
-	 * If alignments are not available, the write is too small,
-	 * or no alignment can be found, fall back to buffered I/O.
+	 * If the file system doesn't advertise any alignment requirements,
+	 * don't try to issue direct I/O.  Fall back to uncached buffered
+	 * I/O if possible because we'll assume it is not block based and
+	 * doesn't need read-modify-write cycles.
 	 */

Not sure what is implied by "not block based" in this comment.

fs/nfsd/vfs.c nfsd_write_dio_iters_init(struct nfsd_file *nf, struct bio_vec *bvec, · line 1310

Quoted 5 lines

fs/nfsd/vfs.c +5 -0

+	/*
+	 * If the I/O is smaller than the larger of the memory and logical
+	 * offset alignment, it is like to require read-modify-write cycles.
+	 * Issue cached buffered I/O.
+	 */

Nit: s/like/likely/ typo.

fs/nfsd/vfs.c nfsd_write_dio_iters_init(struct nfsd_file *nf, struct bio_vec *bvec, · line 1335

Quoted 5 lines

fs/nfsd/vfs.c +2 -0

+	if (!middle ||
+	    ((prefix || suffix) && middle < PAGE_SIZE * 2))
 		goto no_dio;
 
 	if (prefix)

Expressing this threshold in terms of PAGE_SIZE might be a problem for other archs like ARM (where using 64K the norm for performance).

The "Note:" portion of the above comment might do well to address the goal being to avoid using cached buffered for higher order allocations. You or Chuck may have solid ideas for refining wording.

But so you're aware Jon Flynn (who kindly does the heavy lifting of performance testing for me) agrees with you: larger will likely be beneficial but it'll be hardware dependent. So I'm going to expose a knob for Jon to try various values so we can see.

Overall this patch looks good. Would welcome seeing you submit a formal patch. In parallel I'll work with Jon to get this tested on Hammerspace's performance cluster to confirm all looks good.

Quoted 10 lines · Mike Snitzer

fs/nfsd/vfs.c nfsd_write_dio_iters_init(struct nfsd_file *nf, struct bio_vec *bvec, · line 1299

Quoted 8 lines

fs/nfsd/vfs.c +4 -3

-	 * Check if direct I/O is feasible for this write request.
-	 * If alignments are not available, the write is too small,
-	 * or no alignment can be found, fall back to buffered I/O.
+	 * If the file system doesn't advertise any alignment requirements,
+	 * don't try to issue direct I/O.  Fall back to uncached buffered
+	 * I/O if possible because we'll assume it is not block based and
+	 * doesn't need read-modify-write cycles.
 	 */

Not sure what is implied by "not block based" in this comment.

e.g. re-exporting another network file system or tmpfs.

Quoted 7 lines

fs/nfsd/vfs.c nfsd_write_dio_iters_init(struct nfsd_file *nf, struct bio_vec *bvec, · line 1310

Quoted 5 lines

fs/nfsd/vfs.c +5 -0

+	/*
+	 * If the I/O is smaller than the larger of the memory and logical
+	 * offset alignment, it is like to require read-modify-write cycles.
+	 * Issue cached buffered I/O.
+	 */

Nit: s/like/likely/ typo.

Thanks.

Quoted 9 lines

fs/nfsd/vfs.c nfsd_write_dio_iters_init(struct nfsd_file *nf, struct bio_vec *bvec, · line 1335

Quoted 5 lines

fs/nfsd/vfs.c +2 -0

+	if (!middle ||
+	    ((prefix || suffix) && middle < PAGE_SIZE * 2))
 		goto no_dio;
 
 	if (prefix)

Expressing this threshold in terms of PAGE_SIZE might be a problem for other archs like ARM (where using 64K the norm for performance).

True.

Quoted 3 lines

The "Note:" portion of the above comment might do well to address the goal being to avoid using cached buffered for higher order allocations. You or Chuck may have solid ideas for refining wording.

Honestly I'm not worried about higher order allocations at all. Yes, they used to be very painful in the past, but if we get them here it means the file system supports large folios, which means we're going to churn through a lot of them anyway.

Quoted 4 lines

But so you're aware Jon Flynn (who kindly does the heavy lifting of performance testing for me) agrees with you: larger will likely be beneficial but it'll be hardware dependent. So I'm going to expose a knob for Jon to try various values so we can see.

That does sound useful, thanks!

Quoted 3 lines

Overall this patch looks good. Would welcome seeing you submit a formal patch. In parallel I'll work with Jon to get this tested on Hammerspace's performance cluster to confirm all looks good.

I have no problem fixing up the nits and writing a commit message, but I have no time to properly test this. I'm too busy with my own projects and just stealing some time to help with whiteboard coding here :) I'd also be perfectly fine with whoever actually brings this across taking full credit.

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1345

Quoted 7 lines · Chuck Lever

fs/nfsd/vfs.c +7 -0

+no_dio:
+	/*
+	 * No DIO alignment possible - pack into single non-DIO segment.
+	 * IOCB_DONTCACHE preserves the intent of NFSD_IO_DIRECT.
+	 */
+	nfsd_write_dio_seg_init(&segments[0], bvec, nvecs, total, 0,
+				total, iocb);

I'd like to sort out the discussion on why to set IOCB_DONTCACHE when nothing is aligned, but not for the non-aligned parts as that is extremely counter-intuitive. From the other thread it might be because the test case used to justify it is very unaligned and caching partial pages is helpful, but if that is indeed the case the right check would be for writes that are file offset unaligned vs the page or max folio size and not about being able to do parts of it as direct I/O.

Either way a tweak to suddenly use cached buffered I/O when the mode asks for direct should have a comment explaining the justification and explain the rationale instead of rushing it in.

Quoted 17 lines · Christoph Hellwig

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1345

Quoted 7 lines · Chuck Lever

fs/nfsd/vfs.c +7 -0

+no_dio:
+	/*
+	 * No DIO alignment possible - pack into single non-DIO segment.
+	 * IOCB_DONTCACHE preserves the intent of NFSD_IO_DIRECT.
+	 */
+	nfsd_write_dio_seg_init(&segments[0], bvec, nvecs, total, 0,
+				total, iocb);

I'd like to sort out the discussion on why to set IOCB_DONTCACHE when nothing is aligned, but not for the non-aligned parts as that is extremely counter-intuitive. From the other thread it might be because the test case used to justify it is very unaligned and caching partial pages is helpful, but if that is indeed the case the right check would be for writes that are file offset unaligned vs the page or max folio size and not about being able to do parts of it as direct I/O.

Quoted 3 lines

Either way a tweak to suddenly use cached buffered I/O when the mode asks for direct should have a comment explaining the justification and explain the rationale instead of rushing it in.

+1 for an explanatory comment, but I'm not getting what is "counter- intuitive" about leaving the content of the end segments in the page cache. The end segments are small and simply cannot be handled by direct I/O.

I raised a similar concern about whether NFSD needs to care about highly unaligned NFS WRITE requests performing well. I'm not convinced that a performance argument is an appropriate rationale for not using DONTCACHE on the ends.

Upshot is I'm on the fence here; I can see both sides of this controversy.

Those ends lend themselves to benefitting from more capable RMW if/when needed.

All it takes to justify not using DONTCACHE is one workload that benefits (even if suboptimal by nature) given there is no apparent downside (other than requiring we document/comment the behavior).

Or is this something we make tunable?
NFSD_IO_DIRECT_DONTCACHE_UNALIGNED?

Streaming misaligned WRITEs is the only workload I'm aware of where NFSD_IO_DIRECT can be made noticably worse in comparison to NFSD_IO_BUFFERED (but I haven't done a bucnh of small IO testing). That's a pretty good place for NFSD_IO_DIRECT given the fix is a really straightforward tradeoff decision.

Quoted 2 lines · Mike Snitzer

Those ends lend themselves to benefitting from more capable RMW if/when needed.

How are ends of a larger I/O inhently more benefits from cached RMW then say a single tiny I/O?

Quoted 4 lines

All it takes to justify not using DONTCACHE is one workload that benefits (even if suboptimal by nature) given there is no apparent downside (other than requiring we document/comment the behavior).

Well, the entire reason for this patchset is the downside of cached buffered I/O, isn't it? Now suddently there's no downsides?

Also to go back to my previous mail: How can an I/O that is aligned in file offset and length, but not in the backing memory ever going to benefit from this caching?

Quoted 2 lines

Or is this something we make tunable?
NFSD_IO_DIRECT_DONTCACHE_UNALIGNED?

Throwing in yet another tunable while not understanding the problem at hand is about the worst possible outcome.

Quoted 5 lines

Streaming misaligned WRITEs is the only workload I'm aware of where NFSD_IO_DIRECT can be made noticably worse in comparison to NFSD_IO_BUFFERED (but I haven't done a bucnh of small IO testing). That's a pretty good place for NFSD_IO_DIRECT given the fix is a really straightforward tradeoff decision.

Can we please stop talking about "misaligned" as a generic thing, when there is two very clearly different cases of misalignment.

Quoted 6 lines · Christoph Hellwig

Quoted 2 lines · Mike Snitzer

Those ends lend themselves to benefitting from more capable RMW if/when needed.

How are ends of a larger I/O inhently more benefits from cached RMW then say a single tiny I/O?

Any unaligned WRITE, no matter the size of the WRITE, will benefit from RMW of its subpage ends if streaming unaligned WRITEs.

I saw below that even saying "unaligned WRITE" leaves you annoyed.

Every time I say stuff like "the entire reason for the patchset" I set myself up for a rebuttal from you. You just did the same thing. I'm going to try to be accomodating and cool but please lets dial back this weirdly caustic dynamic that is forming yet again.

Why is it that you cannot acknowledge that it isn't an all or nothing situation?

Restating an important point underpinning my desire to handle this niche unaligned streaming WRITE case efficiently: it removes the one case I know of where NFSD_IO_DIRECT cannot compete with NFSD_IO_BUFFERED. Whereby making NFSD_IO_DIRECT a compelling alternative default IO mode. To leave it to use DONTCACHE would be purposely compromising NFSD_IO_DIRECT to be less effective.

I'm also saying: there is very limited downside to using cached buffered for these subpage end segments. But IF I'm proven wrong and they do show themselves to be a problem in the future then a code change will be needed.

Quoted 4 lines

Also to go back to my previous mail: How can an I/O that is aligned in file offset and length, but not in the backing memory ever going to benefit from this caching?

It wouldn't, because it would take the no_dio path and the entire IO would be issued using DONTCACHE.

That I just had to point that out shows how absurd all this contrarian hand-wringing has gotten.

Quoted 6 lines

Quoted 3 lines

Or is this something we make tunable?
NFSD_IO_DIRECT_DONTCACHE_UNALIGNED?

Throwing in yet another tunable while not understanding the problem at hand is about the worst possible outcome.

Just because you don't understand something: that isn't my failing. So your need to be the expert here is misplaced. Please show you understand the problem before you project lack of understanding onto me.

+1 for an explanatory comment, but I'm not getting what is "counter- intuitive" about leaving the content of the end segments in the page cache. The end segments are small and simply cannot be handled by direct I/O.

Quoted 4 lines · Chuck Lever

+1 for an explanatory comment, but I'm not getting what is "counter- intuitive" about leaving the content of the end segments in the page cache. The end segments are small and simply cannot be handled by direct I/O.

The downside if of course extra page cache usage / pollution. If these segments are aligned to the file system minimum I/O size, but not to the memory requirements there is not going to be any RMW cycle that leaving them in the page cache for would be useful.

Similarly if the single segment is not aligned in the file logic space, chances of having another RMW come in are the same as for a large one with unaligned head and/or tail.

From: Mike Snitzer <[email protected]>

When NFSD_IO_DIRECT is selected via the /sys/kernel/debug/nfsd/io_cache_write experimental tunable, split incoming unaligned NFS WRITE requests into a prefix, middle and suffix segment, as needed. The middle segment is now DIO-aligned and the prefix and/or suffix are unaligned. Synchronous buffered IO is used for the unaligned segments, and IOCB_DIRECT is used for the middle DIO-aligned extent.

Although IOCB_DIRECT avoids the use of the page cache, by itself it doesn't guarantee data durability. For UNSTABLE WRITE requests, durability is obtained by a subsequent NFS COMMIT request.

Signed-off-by Mike Snitzer <[email protected]>

Co-developed-by Chuck Lever <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/debugfs.c |   1 +
 fs/nfsd/trace.h   |   1 +
 fs/nfsd/vfs.c     | 140 ++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 138 insertions(+), 4 deletions(-)

fs/nfsd/debugfs.c +1 -0

index 00eb1ecef6ac..7f44689e0a53 100644
@@ -108,6 +108,7 @@ static int nfsd_io_cache_write_set(void *data, u64 val)
 	switch (val) {
 	case NFSD_IO_BUFFERED:
 	case NFSD_IO_DONTCACHE:
+	case NFSD_IO_DIRECT:
 		nfsd_io_cache_write = val;
 		break;
 	default:

fs/nfsd/trace.h +1 -0

index 85a1521ad757..8047a6d97b81 100644
@@ -469,6 +469,7 @@ DEFINE_NFSD_IO_EVENT(read_io_done);
 DEFINE_NFSD_IO_EVENT(read_done);
 DEFINE_NFSD_IO_EVENT(write_start);
 DEFINE_NFSD_IO_EVENT(write_opened);
+DEFINE_NFSD_IO_EVENT(write_direct);
 DEFINE_NFSD_IO_EVENT(write_io_done);
 DEFINE_NFSD_IO_EVENT(write_done);
 DEFINE_NFSD_IO_EVENT(commit_start);

fs/nfsd/vfs.c +81 -0

index 5333d49910d9..7e56be190170 100644
@@ -1254,6 +1254,131 @@ static int wait_for_concurrent_writes(struct file *file)
 	return err;
 }
 
+struct nfsd_write_dio_seg {
+	struct iov_iter			iter;
+	int				flags;
+};
+
+static unsigned long
+iov_iter_bvec_offset(const struct iov_iter *iter)
+{
+	return (unsigned long)(iter->bvec->bv_offset + iter->iov_offset);
+}
+
+static void
+nfsd_write_dio_seg_init(struct nfsd_write_dio_seg *segment,
+			struct bio_vec *bvec, unsigned int nvecs,
+			unsigned long total, size_t start, size_t len,
+			struct kiocb *iocb)
+{
+	iov_iter_bvec(&segment->iter, ITER_SOURCE, bvec, nvecs, total);
+	if (start)
+		iov_iter_advance(&segment->iter, start);
+	iov_iter_truncate(&segment->iter, len);
+	segment->flags = iocb->ki_flags;
+}
+
+static unsigned int
+nfsd_write_dio_iters_init(struct nfsd_file *nf, struct bio_vec *bvec,
+			  unsigned int nvecs, struct kiocb *iocb,
+			  unsigned long total,
+			  struct nfsd_write_dio_seg segments[3])
+{
+	u32 offset_align = nf->nf_dio_offset_align;
+	loff_t prefix_end, orig_end, middle_end;
+	u32 mem_align = nf->nf_dio_mem_align;
+	size_t prefix, middle, suffix;
+	loff_t offset = iocb->ki_pos;
+	unsigned int nsegs = 0;
+
+	/*
+	 * Check if direct I/O is feasible for this write request.
+	 * If alignments are not available, the write is too small,
+	 * or no alignment can be found, fall back to buffered I/O.
+	 */
+	if (unlikely(!mem_align || !offset_align) ||
+	    unlikely(total < max(offset_align, mem_align)))
+		goto no_dio;
+
+	prefix_end = round_up(offset, offset_align);
+	orig_end = offset + total;
+	middle_end = round_down(orig_end, offset_align);
+
+	prefix = prefix_end - offset;
+	middle = middle_end - prefix_end;
+	suffix = orig_end - middle_end;
+
+	if (!middle)
+		goto no_dio;
+
+	if (prefix)
+		nfsd_write_dio_seg_init(&segments[nsegs++], bvec,
+					nvecs, total, 0, prefix, iocb);
+
+	nfsd_write_dio_seg_init(&segments[nsegs], bvec, nvecs,
+				total, prefix, middle, iocb);
+
+	/*
+	 * Check if the bvec iterator is aligned for direct I/O.
+	 *
+	 * bvecs generated from RPC receive buffers are contiguous: After
+	 * the first bvec, all subsequent bvecs start at bv_offset zero
+	 * (page-aligned). Therefore, only the first bvec is checked.
+	 */
+	if (iov_iter_bvec_offset(&segments[nsegs].iter) & (mem_align - 1))
+		goto no_dio;
+	segments[nsegs].flags |= IOCB_DIRECT;
+	nsegs++;
+
+	if (suffix)
+		nfsd_write_dio_seg_init(&segments[nsegs++], bvec, nvecs, total,
+					prefix + middle, suffix, iocb);
+
+	return nsegs;

From: Mike Snitzer <[email protected]>

When NFSD_IO_DIRECT is selected via the /sys/kernel/debug/nfsd/io_cache_write experimental tunable, split incoming unaligned NFS WRITE requests into a prefix, middle and suffix segment, as needed. The middle segment is now DIO-aligned and the prefix and/or suffix are unaligned. Synchronous buffered IO is used for the unaligned segments, and IOCB_DIRECT is used for the middle DIO-aligned extent.

Although IOCB_DIRECT avoids the use of the page cache, by itself it doesn't guarantee data durability. For UNSTABLE WRITE requests, durability is obtained by a subsequent NFS COMMIT request.

Signed-off-by Mike Snitzer <[email protected]>

Co-developed-by Chuck Lever <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/debugfs.c |   1 +
 fs/nfsd/trace.h   |   1 +
 fs/nfsd/vfs.c     | 140 ++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 138 insertions(+), 4 deletions(-)