Re: [PATCHv4 1/1] block: introduce content activity based ioprio

[Zhaoyang Huang]

On Fri, Jan 26, 2024 at 10:24 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Fri, Jan 26, 2024 at 08:08:00PM +0800, zhaoyang.huang wrote:
> > +#ifdef CONFIG_CONTENT_ACT_BASED_IOPRIO
> > +#define bio_add_page(bio, page, len, offset) \
> > +     ({                                      \
> > +             int class, level, hint, activity;       \
> > +             int ret = 0;                            \
> > +             ret = bio_add_page(bio, page, len, offset);             \
> > +             if (ret > 0) {                                          \
> > +                     class = IOPRIO_PRIO_CLASS(bio->bi_ioprio);      \
> > +                     level = IOPRIO_PRIO_LEVEL(bio->bi_ioprio);      \
> > +                     hint = IOPRIO_PRIO_HINT(bio->bi_ioprio);        \
> > +                     activity = IOPRIO_PRIO_ACTIVITY(bio->bi_ioprio);                \
> > +                     activity += (bio->bi_vcnt + 1 <= IOPRIO_NR_ACTIVITY &&          \
> > +                             PageWorkingset(&folio->page)) ? 1 : 0;                  \
>
> I know you didn't even compile this version.
sorry for forgetting to enable corresponding fs, it will be corrected
in the next patchset. The correct version is compiled and verified by
including act_ioprio.h in the below files in the same android
environment as the previous version.

modified:   fs/erofs/zdata.c
modified:   fs/ext4/page-io.c
modified:   fs/ext4/readpage.c
modified:   fs/f2fs/data.c
modified:   fs/mpage.c

>
> More importantly, conceptually it doesn't work.  All kinds of pages
> get added to bios, and not all of them are file/anon pages.  That
> PageWorkingset bit might well be reused for other purposes.  Only
> the caller knows if this is file/anon memory.  You can't do this here.
>
I noticed the none-file bio's you mentioned such as the one in
xfs_rw_bdev() and fscrypt_zeroout_range(). That's also the reason I
don't define the macro in common fs's header file. It should be up to
fs to decide which bio_add_xxx should be replaced by the activity
based one while keeping others as legacy versions.