On Fri, Jan 26, 2024 at 4:55 PM Matthew Wilcox [off-list ref] wrote:

On Fri, Jan 26, 2024 at 03:59:48PM +0800, Zhaoyang Huang wrote:

quoted

loop more mm and fs guys for more comments

I agree with everything Damien said.  But also ...

ok, I will find a way to solve this problem.

quoted

quoted

+bool BIO_ADD_FOLIO(struct bio *bio, struct folio *folio, size_t len,
+               size_t off)

You don't add any users of these functions.  It's hard to assess whether
this is the right API when there are no example users.

Actually, the code has been tested on ext4 and f2fs by patchv2 on a
v6.6 6GB android system where I get the test result posted on the
commit message. These APIs is to keep block layer clean and wrap
things up for fs.

quoted

quoted

+       activity += (bio->bi_vcnt + 1 <= IOPRIO_NR_ACTIVITY &&
+                       PageWorkingset(&folio->page)) ? 1 : 0;

folio_test_workingset().

quoted

quoted

+       return bio_add_page(bio, &folio->page, len, off) > 0;

bio_add_folio().

quoted

quoted

+int BIO_ADD_PAGE(struct bio *bio, struct page *page,
+               unsigned int len, unsigned int offset)
+{
+       int class, level, hint, activity;
+
+       if (bio_add_page(bio, page, len, offset) > 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 <= IOPRIO_NR_ACTIVITY && PageWorkingset(page)) ? 1 : 0;
+               bio->bi_ioprio = IOPRIO_PRIO_VALUE_ACTIVITY(class, level, hint, activity);
+       }

why are BIO_ADD_PAGE and BIO_ADD_FOLIO so very different from each
other?

These two API just repeat the same thing that bio_add_page and
bio_add_folio do.

quoted

quoted

 static __always_inline __u16 ioprio_value(int prioclass, int priolevel,
-                                         int priohint)
+               int priohint)

why did you change this whitespace?

quoted

quoted

 {
        if (IOPRIO_BAD_VALUE(prioclass, IOPRIO_NR_CLASSES) ||
-           IOPRIO_BAD_VALUE(priolevel, IOPRIO_NR_LEVELS) ||
-           IOPRIO_BAD_VALUE(priohint, IOPRIO_NR_HINTS))
+                       IOPRIO_BAD_VALUE(priolevel, IOPRIO_NR_LEVELS) ||
+                       IOPRIO_BAD_VALUE(priohint, IOPRIO_NR_HINTS))

ditto

These white spaces are trimmed by vim, I will change them back in next version.

quoted

quoted

                return IOPRIO_CLASS_INVALID << IOPRIO_CLASS_SHIFT;

        return (prioclass << IOPRIO_CLASS_SHIFT) |
                (priohint << IOPRIO_HINT_SHIFT) | priolevel;
 }
-

more gratuitous whitespace change