On Thu, Oct 12, 2023 at 02:19:02PM +0530, Kanchan Joshi wrote:
> On 10/11/2023 10:22 PM, Bart Van Assche wrote:
> >>> @@ -2926,7 +2926,8 @@ static void bio_set_ioprio(struct bio *bio)
> >>> {
> >>> /* Nobody set ioprio so far? Initialize it based on task's
> >>> nice value */
> >>> if (IOPRIO_PRIO_CLASS(bio->bi_ioprio) == IOPRIO_CLASS_NONE)
> >>> - bio->bi_ioprio = get_current_ioprio();
> >>> + ioprio_set_class_and_level(&bio->bi_ioprio,
> >>> + get_current_ioprio());
> >>> blkcg_set_ioprio(bio);
> >>> }
> >>> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
> >>> index 7578d4f6a969..f2e768ab4b35 100644
> >>> --- a/include/linux/ioprio.h
> >>> +++ b/include/linux/ioprio.h
> >>> @@ -71,4 +71,14 @@ static inline int ioprio_check_cap(int ioprio)
> >>> }
> >>> #endif /* CONFIG_BLOCK */
> >>> +#define IOPRIO_CLASS_LEVEL_MASK ((IOPRIO_CLASS_MASK <<
> >>> IOPRIO_CLASS_SHIFT) | \
> >>> + (IOPRIO_LEVEL_MASK << 0))
> >>> +
> >>> +static inline void ioprio_set_class_and_level(u16 *prio, u16
> >>> class_level)
> >>> +{
> >>> + WARN_ON_ONCE(class_level & ~IOPRIO_CLASS_LEVEL_MASK);
> >>> + *prio &= ~IOPRIO_CLASS_LEVEL_MASK;
> >>> + *prio |= class_level;
> >>
> >> Return of get_current_ioprio() will touch all 16 bits here. So
> >> user-defined value can alter whatever was set in bio by F2FS (patch 4 in
> >> this series). Is that not an issue?
> >
> > The above is incomprehensible to me. Anyway, I will try to answer.
> >
> > It is not clear to me why it is claimed that "get_current_ioprio() will
> > touch all 16 bits here"? The return value of get_current_ioprio() is
> > passed to ioprio_set_class_and_level() and that function clears the hint
> > bits from the get_current_ioprio() return value.
>
> Function does OR bio->bi_ioprio with whatever is the return of
> get_current_ioprio(). So if lifetime bits were set in
> get_current_ioprio(), you will end up setting that in bio->bi_ioprio too.
>
>
> > ioprio_set_class_and_level() preserves the hint bits set by F2FS.
> >
> >> And what is the user interface you have in mind. Is it ioprio based, or
> >> write-hint based or mix of both?
> >
> > Since the data lifetime is encoded in the hint bits, the hint bits need
> > to be set by user space to set a data lifetime.
>
> I asked because more than one way seems to emerge here. Parts of this
> series (Patch 4) are taking inode->i_write_hint (and not ioprio value)
> and putting that into bio.
> I wonder what to expect if application get to send one lifetime with
> fcntl (write-hints) and different one with ioprio. Is that not racy?
Hello Kanchan,
The fcntl F_SET_RW_HINT still exists, which sets inode->i_write_hint.
This is currently only used by f2fs.
The usage of inode->i_write_hint was removed from all filesystems
(except f2fs) in:
c75e707fe1aa ("block: remove the per-bio/request write hint").
This commit also removed bi_write_hint from struct bio.
The fcntl F_SET_FILE_RW_HINT, which used to set f->f_write_hint was removed
in:
7b12e49669c9 ("fs: remove fs.f_write_hint")
This commit also removed f_write_hint from struct file.
My thinking when suggesting to reuse ioprio hints, was that we don't need
to readd write_hint struct members to struct bio and struct request.
SCSI can just reuse the hints bits in ioprio.
Note that my filesystem knowledge is not the best...
But for f2fs, I guess it just needs to set the bio->ioprio hint bits
correctly.
I guess the confusion is if an application does both:
ioprio_set() and fcntl(.., F_SET_RW_HINT, ..), what should the filesystem
use?
Or.. if you use e.g. io_uring to write to a file stored on f2fs...
io_uring has sqe->ioprio, which can contain a write hint, does this get
propagated to the filesystem?
And if so, what if you also did fcntl(.., F_SET_RW_HINT, ..) to set
i_write_hint? Which should the filesystem use to set bio->ioprio?
Kind regards,
Niklas
