Re: [PATCH V8 08/11] block: add blk_end_flush_machinery

[Ming Lei <ming.lei@redhat.com>]

On Sat, Apr 25, 2020 at 10:11:25AM +0200, Christoph Hellwig wrote:
> On Sat, Apr 25, 2020 at 11:44:05AM +0800, Ming Lei wrote:
> > > FYI, we don't really need the bdev to send a bio anymore, this could just
> > > do:
> > > 
> > > 	bio = bio_alloc(GFP_KERNEL, 0); // XXX: shouldn't this be GFP_NOIO??
> > 
> > Error handling.
> 
> bio_alloc does not fail for allocations that can sleep.
> 
> > 
> > > 	bio->bi_disk = rq->rq_disk;
> > > 	bio->bi_partno = 0;
> > > 	bio_associate_blkg(bio); // XXX: actually, shouldn't this come from rq?
> > > 	bio->bi_opf = REQ_OP_WRITE | REQ_PREFLUSH;
> > > 	error = submit_bio_wait(bio);
> > > 	bio_put(bio);
> > 
> > Yeah, that is another way, however I prefer to blkdev_issue_flush
> > because it takes less code and we do have the API of blkdev_issue_flush.
> 
> It is about the same amount of code, and as commented above I think the
> current code uses the wrong blkg assignment.

There can be lots of flush requests which reply on this single flush
emulation, then there can be more associated blkgs, so not sure we can
select a correct one. But this single flush emulation is only triggered
in very unlikey case, does it really matter to select one 'correct' blkcg
for this single flush request without DATA?

BTW, block/blk-flush.c does run aggressive flush merge by the built-in
per-hctx flush request and double queues, and blkcg can't account actual
flush request accurately.

Thanks, 
Ming