Re: [PATCH v2 03/15] block: Support data lifetime in the I/O priority bitfield

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Fri, Oct 13, 2023 at 10:08:29AM +0900, Damien Le Moal wrote:
> On 10/13/23 03:00, Bart Van Assche wrote:
> > On 10/11/23 18:02, Damien Le Moal wrote:
> >> Some have stated interest in CDL in NVMe-oF context, which could
> >> imply that combining CDL and lifetime may be something useful to do
> >> in that space...
> > 
> > We are having this discussion because bi_ioprio is sixteen bits wide and
> > because we don't want to make struct bio larger. How about expanding the
> > bi_ioprio field from 16 to 32 bits and to use separate bits for CDL
> > information and data lifetimes?
> 
> I guess we could do that as well. User side aio_reqprio field of struct aiocb,
> which is used by io_uring and libaio, is an int, so 32-bits also. Changing
> bi_ioprio to match that should not cause regressions or break user space I
> think. Kernel uapi ioprio.h will need some massaging though.
> 
> > This patch does not make struct bio bigger because it changes a three
> > byte hole with a one byte hole:
> 
> Yeah, but if the kernel is compiled with struct randomization, that does not
> really apply, doesn't it ?
> 
> Reading Niklas's reply to Kanchan, I was reminded that using ioprio hint for
> the lifetime may have one drawback: that information will be propagated to the
> device only for direct IOs, no ? For buffered IOs, the information will be
> lost. The other potential disadvantage of the ioprio interface is that we
> cannot define ioprio+hint per file (or per inode really), unlike the old
> write_hint that you initially reintroduced. Are these points blockers for the
> user API you were thinking of ? How do you envision the user specifying
> lifetime ? Per file ? Or are you thinking of not relying on the user to specify
> that but rather the FS (e.g. f2fs) deciding on its own ? If it is the latter, I
> think ioprio+hint is fine (it is simple). But if it is the former, the ioprio
> API may not be the best suited for the job at hand.

Hello Damien,

If you look closer at this series, you will see that even V2 of this series
still uses fcntl F_SET_RW_HINT as the user facing API.

This series simply take the value from fcntl F_SET_RW_HINT
(inode->i_write_hint) and stores it in bio->ioprio.

So it is not really using the ioprio user API.

See the patch to e.g. buffered-io.c:

--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -6,6 +6,7 @@
 #include <linux/module.h>
 #include <linux/compiler.h>
 #include <linux/fs.h>
+#include <linux/fs-lifetime.h>
 #include <linux/iomap.h>
 #include <linux/pagemap.h>
 #include <linux/uio.h>
@@ -1660,6 +1661,7 @@ iomap_alloc_ioend(struct inode *inode, struct iomap_writepage_ctx *wpc,
 			       REQ_OP_WRITE | wbc_to_write_flags(wbc),
 			       GFP_NOFS, &iomap_ioend_bioset);
 	bio->bi_iter.bi_sector = sector;
+	bio_set_data_lifetime(bio, inode->i_write_hint);
 	wbc_init_bio(wbc, bio);
 
 	ioend = container_of(bio, struct iomap_ioend, io_inline_bio);




In commit c75e707fe1aa ("block: remove the per-bio/request write hint")
this line from fs/direct-io.c was removed:
-       bio->bi_write_hint = dio->iocb->ki_hint;

I'm not sure why this series does not readd a similar line to set the
lifetime (using bio_set_data_lifetime()) also for fs/direct-io.c.


I still don't understand what happens if one uses io_uring to write
to a file on a f2fs filesystem using buffered-io, with both
inode->i_write_hint set using fcntl F_SET_RW_HINT, and bits belonging
to life time hints set in the io_uring SQE (sqe->ioprio).

I'm guessing that the:
bio_set_data_lifetime(bio, inode->i_write_hint);
call above in buffered-io.c, will simply overwrite whatever value
that was already stored in bio->ioprio. (Because if I understand
correctly, bio->ioprio will initially be set to the value in the
io_uring SQE (sqe->ioprio).)


Kind regards,
Niklas