[dm-devel] dm-zoned: Avoid metadata flush writeback throttling

[Mikulas Patocka]

On Mon, 11 Sep 2017, Mike Snitzer wrote:

> On Mon, Jul 24 2017 at  5:16am -0400,
> Damien Le Moal <damien.lemoal at wdc.com> wrote:
> 
> > 
> > 
> > On 7/24/17 17:03, Christoph Hellwig wrote:
> > > On Mon, Jul 24, 2017 at 04:44:22PM +0900, Damien Le Moal wrote:
> > >> Avoid metadata flush to be blocked by the writeback throttling code in
> > >> wbt_wait(). Otherwise, we can end up with a deadlock under heavy write
> > >> load with all chunk works active. In such situation, a deadlock can
> > >> happen if the flush work is blocked by the throttling code while holding
> > >> the metadata lock: as the chunk works will wait for the metadata lock
> > >> too, no progress can be made.

Could you explain, what's the specific problem here?

The writeback throttling code is executed at request level and the 
dm-zoned target working above it, at bio level. So I don't see how 
dm-zoned progress could be blocked by writeback throttling.

Maybe you have some other bug in your code and this patch just masks it?

Mikulas

> > >> Signed-off-by: Damien Le Moal <damien.lemoal at wdc.com>
> > >> ---
> > >>  drivers/md/dm-zoned-metadata.c | 3 ++-
> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
> > >> index 884ff7c170a0..f694fb98b002 100644
> > >> --- a/drivers/md/dm-zoned-metadata.c
> > >> +++ b/drivers/md/dm-zoned-metadata.c
> > >> @@ -567,7 +567,8 @@ static void dmz_write_mblock(struct dmz_metadata *zmd, struct dmz_mblock *mblk,
> > >>  	bio->bi_bdev = zmd->dev->bdev;
> > >>  	bio->bi_private = mblk;
> > >>  	bio->bi_end_io = dmz_mblock_bio_end_io;
> > >> -	bio_set_op_attrs(bio, REQ_OP_WRITE, REQ_META | REQ_PRIO);
> > >> +	bio_set_op_attrs(bio, REQ_OP_WRITE,
> > >> +			 REQ_META | REQ_PRIO | REQ_SYNC | REQ_IDLE);
> > > 
> > > Please just assign bi_opf directly instea dof using bio_set_op_attrs
> > > in new code.
> > 
> > OK. Will do.
> > 
> > > Also what do you need REQ_IDLE for?
> > 
> > It is not really needed here, but the wbt_should_throttle() code test is:
> > 
> > if ((bio->bi_opf & (REQ_SYNC | REQ_IDLE)) == (REQ_SYNC | REQ_IDLE))
> > 	return false;
> > 
> > So if REQ_IDLE is not set, wbt will trigger.
> > 
> > Having a req flag that says "no wb throttling please" would be a lot
> > cleaner...
> 
> Did you have a new version of this patch?
> 
> Thanks,
> Mike
>