bcache on XFS: metadata I/O (dirent I/O?) not getting cached at all?

DISCUSSION Nix 4 participants 14 messages Feb 6, 2019 – Feb 7, 2019 bcache (block layer cache)

Share & tools

Nix

author

2019-02-06 22:11 UTC #0 lore raw

So I just upgraded to 4.20 and revived my long-turned-off bcache now that the metadata corruption leading to mount failure on dirty close may have been identified (applying Tang Junhui's patch to do so)... and I spotted something a bit disturbing. It appears that XFS directory and metadata I/O is going more or less entirely uncached.

Here's some bcache stats before and after a git status of a *huge* uncached tree (Chromium) on my no-writeback readaround cache. It takes many minutes and pounds the disk with massively seeky metadata I/O in the process:

Before:

stats_total/bypassed: 48.3G
stats_total/cache_bypass_hits: 7942
stats_total/cache_bypass_misses: 861045
stats_total/cache_hit_ratio: 3
stats_total/cache_hits: 16286
stats_total/cache_miss_collisions: 25
stats_total/cache_misses: 411575
stats_total/cache_readaheads: 0

After:
stats_total/bypassed: 49.3G
stats_total/cache_bypass_hits: 7942
stats_total/cache_bypass_misses: 1154887
stats_total/cache_hit_ratio: 3
stats_total/cache_hits: 16291
stats_total/cache_miss_collisions: 25
stats_total/cache_misses: 411625
stats_total/cache_readaheads: 0

Huge increase in bypassed reads, essentially no new cached reads. This is... basically the optimum case for bcache, and it's not caching it!

From my reading of xfs_dir2_leaf_readbuf(), it looks like essentially all directory reads in XFS appear to bcache as a single non-readahead followed by a pile of readahead I/O: bcache bypasses readahead bios, so all directory reads (or perhaps all directory reads larger than a single block) are going to be bypassed out of hand.

This seems... suboptimal, but so does filling up the cache with read-ahead blocks (particularly for non-metadata) that are never used. Anyone got any ideas, 'cos I'm currently at a loss: XFS doesn't appear to let us distinguish between "read-ahead just in case but almost certain to be accessed" (like directory blocks) and "read ahead on the offchance because someone did a single-block file read and what the hell let's suck in a bunch more".

As it is, this seems to render bcache more or less useless with XFS, since bcache's primary raison d'etre is precisely to cache seeky stuff like metadata. :(

Dave Chinner

2019-02-06 23:43 UTC #1 lore raw

Quoted 32 lines · Nix

So I just upgraded to 4.20 and revived my long-turned-off bcache now that the metadata corruption leading to mount failure on dirty close may have been identified (applying Tang Junhui's patch to do so)... and I spotted something a bit disturbing. It appears that XFS directory and metadata I/O is going more or less entirely uncached.
Here's some bcache stats before and after a git status of a *huge* uncached tree (Chromium) on my no-writeback readaround cache. It takes many minutes and pounds the disk with massively seeky metadata I/O in the process:
Before:
stats_total/bypassed: 48.3G
stats_total/cache_bypass_hits: 7942
stats_total/cache_bypass_misses: 861045
stats_total/cache_hit_ratio: 3
stats_total/cache_hits: 16286
stats_total/cache_miss_collisions: 25
stats_total/cache_misses: 411575
stats_total/cache_readaheads: 0
After:
stats_total/bypassed: 49.3G
stats_total/cache_bypass_hits: 7942
stats_total/cache_bypass_misses: 1154887
stats_total/cache_hit_ratio: 3
stats_total/cache_hits: 16291
stats_total/cache_miss_collisions: 25
stats_total/cache_misses: 411625
stats_total/cache_readaheads: 0

Quoted 8 lines

Huge increase in bypassed reads, essentially no new cached reads. This is... basically the optimum case for bcache, and it's not caching it!
From my reading of xfs_dir2_leaf_readbuf(), it looks like essentially all directory reads in XFS appear to bcache as a single non-readahead followed by a pile of readahead I/O: bcache bypasses readahead bios, so all directory reads (or perhaps all directory reads larger than a single block) are going to be bypassed out of hand.

That's a bcache problem, not an XFS problem. XFS does extensive amounts of metadata readahead (btree traversals, directory access, etc), and always has.

If bcache considers readahead as "not worth caching" then that has nothing to do with XFS.

Quoted 3 lines

This seems... suboptimal, but so does filling up the cache with read-ahead blocks (particularly for non-metadata) that are never used.

Which is not the case for XFS. We do readahead when we know we are going to need a block in the near future. It is rarely unnecessary, it's a mechanism to reduce access latency when we do need to access the metadata.

Quoted 5 lines

Anyone got any ideas, 'cos I'm currently at a loss: XFS doesn't appear to let us distinguish between "read-ahead just in case but almost certain to be accessed" (like directory blocks) and "read ahead on the offchance because someone did a single-block file read and what the hell let's suck in a bunch more".

File data readahead: REQ_RAHEAD
Metadata readahead: REQ_META | REQ_RAHEAD

drivers/md/bcache/request.c::check_should_bypass():

        /*
         * Flag for bypass if the IO is for read-ahead or background,
         * unless the read-ahead request is for metadata (eg, for gfs2).
         */
        if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) &&
            !(bio->bi_opf & REQ_PRIO))
                goto skip;

bcache needs fixing - it thinks REQ_PRIO means metadata IO. That's wrong - REQ_META means it's metadata IO, and so this is a bcache bug.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Andre Noll

2019-02-07 00:24 UTC #2 lore raw

On Thu, Feb 07, 10:43, Dave Chinner wrote

Quoted 13 lines

File data readahead: REQ_RAHEAD
Metadata readahead: REQ_META | REQ_RAHEAD

drivers/md/bcache/request.c::check_should_bypass():

        /*
         * Flag for bypass if the IO is for read-ahead or background,
         * unless the read-ahead request is for metadata (eg, for gfs2).
         */
        if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) &&
            !(bio->bi_opf & REQ_PRIO))
                goto skip;

Quoted 3 lines

bcache needs fixing - it thinks REQ_PRIO means metadata IO. That's wrong - REQ_META means it's metadata IO, and so this is a bcache bug.

Do you think 752f66a75abad is bad (ha!) and should be reverted?

Thanks
Andre

Signature · 3 lines

Max Planck Institute for Developmental Biology
Max-Planck-Ring 5, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

Dave Chinner

2019-02-07 02:26 UTC #3 lore raw

Quoted 14 lines · Andre Noll

On Thu, Feb 07, 10:43, Dave Chinner wrote
Quoted 13 lines
File data readahead: REQ_RAHEAD
Metadata readahead: REQ_META | REQ_RAHEAD
drivers/md/bcache/request.c::check_should_bypass():
        /*
         * Flag for bypass if the IO is for read-ahead or background,
         * unless the read-ahead request is for metadata (eg, for gfs2).
         */
        if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) &&
            !(bio->bi_opf & REQ_PRIO))
                goto skip;

Quoted 5 lines

Quoted 3 lines
bcache needs fixing - it thinks REQ_PRIO means metadata IO. That's wrong - REQ_META means it's metadata IO, and so this is a bcache bug.
Do you think 752f66a75abad is bad (ha!) and should be reverted?

Yes, that change is just broken. From include/linux/blk_types.h:

__REQ_META, /* metadata io request */
__REQ_PRIO, /* boost priority in cfq */

i.e. REQ_META means that it is a metadata request, REQ_PRIO means it is a "high priority" request. Two completely different things, often combined, but not interchangeable.

So, yeah, that needs to be reverted if you want bcache to function properly for metadata caching.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Coly Li

2019-02-07 02:27 UTC #8 lore raw

In reply to Andre Noll

Quoted 11 lines · Andre Noll

On Thu, Feb 07, 10:43, Dave Chinner wrote
Quoted 10 lines
File data readahead: REQ_RAHEAD Metadata readahead: REQ_META | REQ_RAHEAD
drivers/md/bcache/request.c::check_should_bypass():
/* * Flag for bypass if the IO is for read-ahead or background, * unless the read-ahead request is for metadata (eg, for gfs2). */ if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) && !(bio->bi_opf & REQ_PRIO)) goto skip;

Quoted 5 lines

Quoted 3 lines
bcache needs fixing - it thinks REQ_PRIO means metadata IO. That's wrong - REQ_META means it's metadata IO, and so this is a bcache bug.
Do you think 752f66a75abad is bad (ha!) and should be reverted?

Hi Dave and Andre,

Correct me if I am wrong: REQ_META is used for blktrace to tag metadata IO, and REQ_PRIO is used for block layer to handle metadata IO.

I discussed with Christoph Hellwig about this topic quite long time ago, and got the above conclusion.

If different file system handles metadata flags in unified ways, it is OK to me to change the code to: !(bio->bi_opf & (REQ_META |REQ_PRIO)).

Thanks in advance.

Signature · 1 line

Coly Li

Coly Li

2019-02-07 02:38 UTC #4 lore raw

In reply to Dave Chinner

Quoted 19 lines · Dave Chinner

Quoted 18 lines · Andre Noll
On Thu, Feb 07, 10:43, Dave Chinner wrote
Quoted 16 lines
File data readahead: REQ_RAHEAD
Metadata readahead: REQ_META | REQ_RAHEAD
drivers/md/bcache/request.c::check_should_bypass():
        /*
         * Flag for bypass if the IO is for read-ahead or background,
         * unless the read-ahead request is for metadata (eg, for gfs2).
         */
        if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) &&
            !(bio->bi_opf & REQ_PRIO))
                goto skip;
bcache needs fixing - it thinks REQ_PRIO means metadata IO. That's wrong - REQ_META means it's metadata IO, and so this is a bcache bug.

Quoted 8 lines

Quoted 1 line
Do you think 752f66a75abad is bad (ha!) and should be reverted?
Yes, that change is just broken. From include/linux/blk_types.h:
__REQ_META, /* metadata io request */
__REQ_PRIO, /* boost priority in cfq */

Hi Dave,

Quoted 3 lines

i.e. REQ_META means that it is a metadata request, REQ_PRIO means it is a "high priority" request. Two completely different things, often combined, but not interchangeable.

I found in file system metadata IO, most of time REQ_META and REQ_PRIO are tagged together for bio, but XFS seems not use REQ_PRIO.

Is there any basic principle for when should these tags to be used or not ? e.g. If REQ_META is enough for meta data I/O, why REQ_PRIO is used too. And if REQ_PRIO is necessary, why it is not used in fs/xfs/ code ?

Quoted 3 lines

So, yeah, that needs to be reverted if you want bcache to function properly for metadata caching.

Sure, I will fix this, once I make it clear to me.

Thanks for the hint.

Signature · 1 line

Coly Li

Dave Chinner

2019-02-07 03:10 UTC #5 lore raw

Quoted 29 lines · Coly Li

Quoted 19 lines · Dave Chinner
Quoted 18 lines · Andre Noll
On Thu, Feb 07, 10:43, Dave Chinner wrote
Quoted 16 lines
File data readahead: REQ_RAHEAD
Metadata readahead: REQ_META | REQ_RAHEAD
drivers/md/bcache/request.c::check_should_bypass():
        /*
         * Flag for bypass if the IO is for read-ahead or background,
         * unless the read-ahead request is for metadata (eg, for gfs2).
         */
        if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) &&
            !(bio->bi_opf & REQ_PRIO))
                goto skip;
bcache needs fixing - it thinks REQ_PRIO means metadata IO. That's wrong - REQ_META means it's metadata IO, and so this is a bcache bug.
Quoted 8 lines
Quoted 1 line
Do you think 752f66a75abad is bad (ha!) and should be reverted?
Yes, that change is just broken. From include/linux/blk_types.h:
__REQ_META, /* metadata io request */
__REQ_PRIO, /* boost priority in cfq */

Quoted 8 lines

Hi Dave,
Quoted 3 lines
i.e. REQ_META means that it is a metadata request, REQ_PRIO means it is a "high priority" request. Two completely different things, often combined, but not interchangeable.
I found in file system metadata IO, most of time REQ_META and REQ_PRIO are tagged together for bio, but XFS seems not use REQ_PRIO.

Yes, that's correct, because we don't specifically prioritise metadata IO over data IO.

Quoted 2 lines

Is there any basic principle for when should these tags to be used or not ?

Yes.

Quoted 2 lines

e.g. If REQ_META is enough for meta data I/O, why REQ_PRIO is used too.

REQ_META is used for metadata. REQ_PRIO is used to communicate to the lower layers that the submitter considers this IO to be more important that non REQ_PRIO IO and so dispatch should be expedited.

IOWs, if the filesystem considers metadata IO to be more important that user data IO, then it will use REQ_PRIO | REQ_META rather than just REQ_META.

Historically speaking, REQ_PRIO was a hack for CFQ to get it to dispatch journal IO from a different thread without waiting for a time slice to expire. In the XFs world, we just said "don't use CFQ, it's fundametnally broken for highly concurrent applications" and didn't bother trying to hack around the limitations of CFQ.

These days REQ_PRIO is only used by the block layer writeback throttle, but unlike bcache it considers both REQ_META and REQ_PRIO to mean the same thing.

REQ_META, OTOH, is used by BFQ and blk-cgroup to detect metadata IO and don't look at REQ_PRIO at all. So, really, REQ_META is for metadata, not REQ_PRIO. REQ_PRIO looks like it should just go away.

Quoted 1 line

And if REQ_PRIO is necessary, why it is not used in fs/xfs/ code ?

It's not necessary, it's just an /optimisation/ that some filesystems make and some IO schedulers used to pay attention to. It looks completely redundant now.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Coly Li

2019-02-07 08:16 UTC #10 lore raw

In reply to Nix

Quoted 49 lines · Nix

So I just upgraded to 4.20 and revived my long-turned-off bcache now that the metadata corruption leading to mount failure on dirty close may have been identified (applying Tang Junhui's patch to do so)... and I spotted something a bit disturbing. It appears that XFS directory and metadata I/O is going more or less entirely uncached.
Here's some bcache stats before and after a git status of a *huge* uncached tree (Chromium) on my no-writeback readaround cache. It takes many minutes and pounds the disk with massively seeky metadata I/O in the process:
Before:
stats_total/bypassed: 48.3G
stats_total/cache_bypass_hits: 7942
stats_total/cache_bypass_misses: 861045
stats_total/cache_hit_ratio: 3
stats_total/cache_hits: 16286
stats_total/cache_miss_collisions: 25
stats_total/cache_misses: 411575
stats_total/cache_readaheads: 0
After:
stats_total/bypassed: 49.3G
stats_total/cache_bypass_hits: 7942
stats_total/cache_bypass_misses: 1154887
stats_total/cache_hit_ratio: 3
stats_total/cache_hits: 16291
stats_total/cache_miss_collisions: 25
stats_total/cache_misses: 411625
stats_total/cache_readaheads: 0
Huge increase in bypassed reads, essentially no new cached reads. This is... basically the optimum case for bcache, and it's not caching it!
From my reading of xfs_dir2_leaf_readbuf(), it looks like essentially all directory reads in XFS appear to bcache as a single non-readahead followed by a pile of readahead I/O: bcache bypasses readahead bios, so all directory reads (or perhaps all directory reads larger than a single block) are going to be bypassed out of hand.
This seems... suboptimal, but so does filling up the cache with read-ahead blocks (particularly for non-metadata) that are never used. Anyone got any ideas, 'cos I'm currently at a loss: XFS doesn't appear to let us distinguish between "read-ahead just in case but almost certain to be accessed" (like directory blocks) and "read ahead on the offchance because someone did a single-block file read and what the hell let's suck in a bunch more".

Quoted 4 lines

As it is, this seems to render bcache more or less useless with XFS, since bcache's primary raison d'etre is precisely to cache seeky stuff like metadata. :(

Hi Nix,

Could you please to try whether the attached patch makes things better ?

Thanks in advance for your help.

Signature · 1 line

Coly Li

From 7c27e26017f6297a6bc6a8075732f69d3edcc52e Mon Sep 17 00:00:00 2001
From: Coly Li <[email protected]>
Date: Thu, 7 Feb 2019 15:54:24 +0800
Subject: [PATCH] bcache: use (REQ_META|REQ_PRIO) to indicate bio for metadata

In 'commit 752f66a75aba ("bcache: use REQ_PRIO to indicate bio for metadata")' REQ_META is replaced by REQ_PRIO to indicate metadata bio. This assumption is not always correct, e.g. XFS uses REQ_META to mark metadata bio other than REQ_PRIO. This is why Nix reports a regression that bcache does not cache metadata for XFS after the above commit.

Thanks to Dave Chinner, he explains the difference between REQ_META and REQ_PRIO from view of file system developer. Here I quote part of his explanation from mailing list,
REQ_META is used for metadata. REQ_PRIO is used to communicate to the lower layers that the submitter considers this IO to be more important that non REQ_PRIO IO and so dispatch should be expedited.

IOWs, if the filesystem considers metadata IO to be more important that user data IO, then it will use REQ_PRIO | REQ_META rather than just REQ_META.

Then it seems bios with REQ_META or REQ_PRIO should both be cached for performance optimation, because they are all probably low I/O latency demand by upper layer (e.g. file system).

So in this patch, when we want to check whether a bio is metadata related, REQ_META and REQ_PRIO are both checked. Then both metadata and high priority I/O requests will be handled properly.

Reported-by Nix <[email protected]>

Signed-off-by Coly Li <[email protected]>

Cc Dave Chinner <[email protected]>

Cc Andre Noll <[email protected]>

Cc Christoph Hellwig <[email protected]>

---
 drivers/md/bcache/request.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/md/bcache/request.c +2 -2

index 3bf35914bb57..62bda90a38dc 100644
@@ -395,7 +395,7 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	 * unless the read-ahead request is for metadata (eg, for gfs2 or xfs).
 	 */
 	if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) &&
-	    !(bio->bi_opf & REQ_PRIO))
+	    !(bio->bi_opf & (REQ_META|REQ_PRIO)))
 		goto skip;
 
 	if (bio->bi_iter.bi_sector & (c->sb.block_size - 1) ||

@@ -877,7 +877,7 @@ static int cached_dev_cache_miss(struct btree *b, struct search *s,
 	}
 
 	if (!(bio->bi_opf & REQ_RAHEAD) &&
-	    !(bio->bi_opf & REQ_PRIO) &&
+	    !(bio->bi_opf & (REQ_META|REQ_PRIO)) &&
 	    s->iop.c->gc_stats.in_use < CUTOFF_CACHE_READA)
 		reada = min_t(sector_t, dc->readahead >> 9,
 			      get_capacity(bio->bi_disk) - bio_end_sector(bio));

Signature · 1 line

2.16.4

Coly Li

2019-02-07 08:18 UTC #6 lore raw

In reply to Dave Chinner

Quoted 72 lines · Dave Chinner

Quoted 29 lines · Coly Li
Quoted 19 lines · Dave Chinner
Quoted 18 lines · Andre Noll
On Thu, Feb 07, 10:43, Dave Chinner wrote
Quoted 16 lines
File data readahead: REQ_RAHEAD
Metadata readahead: REQ_META | REQ_RAHEAD
drivers/md/bcache/request.c::check_should_bypass():
        /*
         * Flag for bypass if the IO is for read-ahead or background,
         * unless the read-ahead request is for metadata (eg, for gfs2).
         */
        if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) &&
            !(bio->bi_opf & REQ_PRIO))
                goto skip;
bcache needs fixing - it thinks REQ_PRIO means metadata IO. That's wrong - REQ_META means it's metadata IO, and so this is a bcache bug.
Quoted 8 lines
Quoted 1 line
Do you think 752f66a75abad is bad (ha!) and should be reverted?
Yes, that change is just broken. From include/linux/blk_types.h:
__REQ_META, /* metadata io request */
__REQ_PRIO, /* boost priority in cfq */
Quoted 8 lines
Hi Dave,
Quoted 3 lines
i.e. REQ_META means that it is a metadata request, REQ_PRIO means it is a "high priority" request. Two completely different things, often combined, but not interchangeable.
I found in file system metadata IO, most of time REQ_META and REQ_PRIO are tagged together for bio, but XFS seems not use REQ_PRIO.
Yes, that's correct, because we don't specifically prioritise metadata IO over data IO.
Quoted 2 lines
Is there any basic principle for when should these tags to be used or not ?
Yes.
Quoted 2 lines
e.g. If REQ_META is enough for meta data I/O, why REQ_PRIO is used too.
REQ_META is used for metadata. REQ_PRIO is used to communicate to the lower layers that the submitter considers this IO to be more important that non REQ_PRIO IO and so dispatch should be expedited.
IOWs, if the filesystem considers metadata IO to be more important that user data IO, then it will use REQ_PRIO | REQ_META rather than just REQ_META.
Historically speaking, REQ_PRIO was a hack for CFQ to get it to dispatch journal IO from a different thread without waiting for a time slice to expire. In the XFs world, we just said "don't use CFQ, it's fundametnally broken for highly concurrent applications" and didn't bother trying to hack around the limitations of CFQ.
These days REQ_PRIO is only used by the block layer writeback throttle, but unlike bcache it considers both REQ_META and REQ_PRIO to mean the same thing.
REQ_META, OTOH, is used by BFQ and blk-cgroup to detect metadata IO and don't look at REQ_PRIO at all. So, really, REQ_META is for metadata, not REQ_PRIO. REQ_PRIO looks like it should just go away.

Quoted 5 lines

Quoted 1 line
And if REQ_PRIO is necessary, why it is not used in fs/xfs/ code ?
It's not necessary, it's just an /optimisation/ that some filesystems make and some IO schedulers used to pay attention to. It looks completely redundant now.

Hi Dave,

Thanks for your detailed explanation. This is great hint from view of file system developer :-)

I just compose a fix, hope it makes better.

Signature · 1 line

Coly Li

Andre Noll

2019-02-07 09:28 UTC #9 lore raw

In reply to Coly Li

On Thu, Feb 07, 10:27, Coly Li wrote

Quoted 2 lines

If different file system handles metadata flags in unified ways, it is OK to me to change the code to: !(bio->bi_opf & (REQ_META |REQ_PRIO)).

Yes, that's the smallest fix that should also go into 4.19-stable.

In the long run, we should try to get rid of the 45 instances of REQ_PRIO. Most users specify REQ_META | REQ_PRIO anyway, which leaves only a few other instances to look at.

I think the one in submit_bh_wbc() of fs/buffer.c can just be removed while block/cfq-iosched.c does not use REQ_META at all, so the simple s/REQ_PRIO/REQ_META should be OK. drivers/staging/erofs/data.c is also easy to fix.

Best
Andre

Signature · 3 lines

Max Planck Institute for Developmental Biology
Max-Planck-Ring 5, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

Andre Noll

2019-02-07 09:41 UTC #11 lore raw

In reply to Coly Li

On Thu, Feb 07, 16:16, Coly Li wrote

Quoted 9 lines

From: Coly Li <[email protected]>
Date: Thu, 7 Feb 2019 15:54:24 +0800
Subject: [PATCH] bcache: use (REQ_META|REQ_PRIO) to indicate bio for metadata
In 'commit 752f66a75aba ("bcache: use REQ_PRIO to indicate bio for metadata")' REQ_META is replaced by REQ_PRIO to indicate metadata bio. This assumption is not always correct, e.g. XFS uses REQ_META to mark metadata bio other than REQ_PRIO. This is why Nix reports a regression that bcache does not cache metadata for XFS after the above commit.

maybe s/reports a regression/noticed

Quoted 11 lines

Thanks to Dave Chinner, he explains the difference between REQ_META and REQ_PRIO from view of file system developer. Here I quote part of his explanation from mailing list,
REQ_META is used for metadata. REQ_PRIO is used to communicate to the lower layers that the submitter considers this IO to be more important that non REQ_PRIO IO and so dispatch should be expedited.
IOWs, if the filesystem considers metadata IO to be more important that user data IO, then it will use REQ_PRIO | REQ_META rather than just REQ_META.

Quoted 7 lines

Then it seems bios with REQ_META or REQ_PRIO should both be cached for performance optimation, because they are all probably low I/O latency demand by upper layer (e.g. file system).
So in this patch, when we want to check whether a bio is metadata related, REQ_META and REQ_PRIO are both checked. Then both metadata and high priority I/O requests will be handled properly.

s/check whether a bio is metadata related/decide whether to bypass the cache

Apart from these two nitpicks, feel free to add my Reviewed-by.

Thanks
Andre

Signature · 3 lines

Max Planck Institute for Developmental Biology
Max-Planck-Ring 5, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

Coly Li

2019-02-07 10:23 UTC #12 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Quoted 34 lines · Andre Noll

On Thu, Feb 07, 16:16, Coly Li wrote
Quoted 10 lines
From: Coly Li <[email protected]> Date: Thu, 7 Feb 2019 15:54:24 +0800 Subject: [PATCH] bcache: use (REQ_META|REQ_PRIO) to indicate bio for metadata
In 'commit 752f66a75aba ("bcache: use REQ_PRIO to indicate bio for metadata")' REQ_META is replaced by REQ_PRIO to indicate metadata bio. This assumption is not always correct, e.g. XFS uses REQ_META to mark metadata bio other than REQ_PRIO. This is why Nix reports a regression that bcache does not cache metadata for XFS after the above commit.
maybe s/reports a regression/noticed
Quoted 11 lines
Thanks to Dave Chinner, he explains the difference between REQ_META and REQ_PRIO from view of file system developer. Here I quote part of his explanation from mailing list, REQ_META is used for metadata. REQ_PRIO is used to communicate to the lower layers that the submitter considers this IO to be more important that non REQ_PRIO IO and so dispatch should be expedited.
IOWs, if the filesystem considers metadata IO to be more important that user data IO, then it will use REQ_PRIO | REQ_META rather than just REQ_META.
Quoted 8 lines
Then it seems bios with REQ_META or REQ_PRIO should both be cached for performance optimation, because they are all probably low I/O latency demand by upper layer (e.g. file system).
So in this patch, when we want to check whether a bio is metadata related, REQ_META and REQ_PRIO are both checked. Then both metadata and high priority I/O requests will be handled properly.

Quoted 4 lines

s/check whether a bio is metadata related/decide whether to bypass the cache
Apart from these two nitpicks, feel free to add my Reviewed-by.

Hi Andre,

Thanks for your review :-)

- --

Coly Li
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE6j5FL/T5SGCN6PrQxzkHk2t9+PwFAlxcBy0ACgkQxzkHk2t9 +PxmeRAAnIh3KzGx13lmpj/0sDlellc02eawvooSDE8zTNsqo/UzqAB1lzUvtJx1 SQXYepoM4hPjrGzDKBe+oaMWuzG6WO08kI0dMT1Q1cfKX2eucExa8G9oRHQqxxBq wRChl4HKScqvyCy0XTQYet8QJwFAeBAdBzM2L0VOuulsRjLKBSfpcH+jr9fdNt/S wWGv9KVlJDq3BZrRA0TA1ovDl4wohyvtXM4OL27ahUMlmj4cI0XlaoketaAVM8QS z7uK+9sRdemW3cvfkl76wfIkiSN43Pk6RZcKRAHzlSgwF1L3xY7ZPxhFz187bFzI ANpTLVXiv1JNx+jnceV4O7UVZyj06hQhh1VFLedqpAVvzgYNp8RRSaiWucNH7m5d 6qzw5+ob5jMEI2vdXooYC8hVwNaXa+3XfVU5QLlKVG9Al1kZxgkE3e8s28pjj3Df WJXaqkOCnal12FZ9Hz+Ev/Jp/1wmN3TagPJyeVlc/0vQTIXfBFuSssL+n/1RrKjd wTc2WuWBf2x7BLnuCX6z4fDvPLuW8LOJCrCYsqr4z5s7YI6mb0B4qDSqli8dN2TB vxnY1K1WQW/9BhE/QQpXHKBD2nRbPYcaAZEhk3UvIdV2dvafdQpcD4FgrEXGKXV8 MmkaeTcQ4NcSNvGWgG9vaiY0acE8YGtqNsBDAtjOglJrP/tJjJA= =kG5J
-----END PGP SIGNATURE-----

Nix

author

2019-02-07 13:10 UTC #7 lore raw

In reply to Coly Li

On 7 Feb 2019, Coly Li stated:

Quoted 5 lines · Dave Chinner

Quoted 2 lines
So, yeah, that needs to be reverted if you want bcache to function properly for metadata caching.
Sure, I will fix this, once I make it clear to me.

I'll give it a test :)

The meaning of these flags was somewhat opaque to me, too (mostly due to
novelty: I've never really looked at anything in the block layer before).

Signature · 1 line

NULL && (void)

Nix

author

2019-02-07 20:51 UTC #13 lore raw

In reply to Coly Li

On 7 Feb 2019, Coly Li uttered the following:

Quoted 8 lines · Nix

Quoted 3 lines
As it is, this seems to render bcache more or less useless with XFS, since bcache's primary raison d'etre is precisely to cache seeky stuff like metadata. :(
Hi Nix,
Could you please to try whether the attached patch makes things better ?

Looks good! Before huge tree cp -al:

loom:~# bcache-stats 
stats_total/bypassed: 1.0G
stats_total/cache_bypass_hits: 16
stats_total/cache_bypass_misses: 26436
stats_total/cache_hit_ratio: 46
stats_total/cache_hits: 24349
stats_total/cache_miss_collisions: 8
stats_total/cache_misses: 27898
stats_total/cache_readaheads: 0

After:

stats_total/bypassed: 1.1G
stats_total/cache_bypass_hits: 16
stats_total/cache_bypass_misses: 27176
stats_total/cache_hit_ratio: 43
stats_total/cache_hits: 24443
stats_total/cache_miss_collisions: 9
stats_total/cache_misses: 32152 <----
stats_total/cache_readaheads: 0

So loads of new misses. (A bunch of bypassed misses too. Not sure where those came from, maybe some larger sequential reads somewhere, but a lot is getting cached now, and every bit of metadata that gets cached means things get a bit faster.)

btw I have ported ewheeler's ioprio-based cache hinting patch to 4.20; I/O below the ioprio threshold bypasses everything, even metadata and REQ_PRIO stuff. It was trivial, but I was able to spot and fix a tiny bypass accounting bug in the patch in the process): see
http://www.esperi.org.uk/~nix/bundles/bcache-ioprio.bundle. (I figured you didn't want almost exactly the same patch series as before posted to the list, but I can do that if you prefer.)

Put this all together and it seems to work very well: my test massive compile triggered 500MiB of metadata writes at the start and then the actual compile (being entirely sequential reads) hardly wrote anything out and was almost entirely bypassed: meanwhile a huge git push I ran at idle priority didn't pollute the cache at all. Excellent!

(I'm also keeping write volumes down by storing transient things like objdirs that just sit in the page cache and then get deleted on a separate non-bcached, non-journalled ext4 fs at the start of the the spinning rust disk, with subdirs of this fs bind-mounted into various places as needed. I should make the scripts that do that public because they seem likely to be useful to bcache users...)

Semi-unrelated side note: after my most recent reboot, which involved a bcache journal replay even though my shutdown was clean, the stats_total reset; the cache device's bcache/written and bcache/set/cache_available_percent also flipped to 0 and 100%,. I suspect this is merely a stats bug of some sort, because the boot was notably faster than before and cache_hits was about 6000 by the time it was done. bcache/priority_stats *does* say that the cache is "only" 98% unused, like it did before. Maybe cache_available_percent doesn't mean what I thought it did.

Signature · 1 line

NULL && (void)