Re: [PATCH v2 01/15] block: Make bio_set_ioprio() modify fewer bio->bi_ioprio bits

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

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