On Fri, Jun 16, 2017 at 05:53:12AM +0800, Anand Jain wrote:
> On 06/16/2017 12:49 AM, David Sterba wrote:
> > For devices that support flushing, we allocate a bio, submit, wait for
> > it and then free it. The bio allocation does not fail so ENOMEM is not a
> > problem but we still may unnecessarily stress the allocation subsystem.
> >
> > Instead, we can allocate the device at the same time we allocate the
> > device and reuse it each time we need to flush the barriers. The bio is
> > reset before each use. Reference counting is simplified to just device
> > allocation (get) and freeing (put).
> >
> > Note for write_dev_flush: we check the queue flush status again as we
> > can't use the existence of bio as before.
>
> Looks good few items as below..
>
> > Signed-off-by: David Sterba <dsterba@xxxxxxxx>
> > ---
> > fs/btrfs/disk-io.c | 24 ++++++------------------
> > fs/btrfs/volumes.c | 12 ++++++++++++
> > 2 files changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 2b00ebff13f8..27d44d6ab775 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -3482,9 +3482,7 @@ static int write_dev_supers(struct btrfs_device *device,
> > */
> > static void btrfs_end_empty_barrier(struct bio *bio)
> > {
> > - if (bio->bi_private)
> > - complete(bio->bi_private);
> > - bio_put(bio);
> > + complete(bio->bi_private);
> > }
> >
> > /*
> > @@ -3494,26 +3492,19 @@ static void btrfs_end_empty_barrier(struct bio *bio)
> > static void write_dev_flush(struct btrfs_device *device)
> > {
> > struct request_queue *q = bdev_get_queue(device->bdev);
> > - struct bio *bio;
> > + struct bio *bio = device->flush_bio;
> >
> > if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags))
> > return;
> >
> > - /*
> > - * one reference for us, and we leave it for the
> > - * caller
> > - */
> > - device->flush_bio = NULL;
> > - bio = btrfs_io_bio_alloc(0);
> > + bio_reset(bio);
> > bio->bi_end_io = btrfs_end_empty_barrier;
> > bio->bi_bdev = device->bdev;
> > bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_PREFLUSH;
> > init_completion(&device->flush_wait);
> > bio->bi_private = &device->flush_wait;
> > - device->flush_bio = bio;
> >
> > - bio_get(bio);
> > - btrfsic_submit_bio(bio);
> > + submit_bio(bio);
>
> Originally it went through the btrfsic. There is no mention
> of this change if its not an oversight.
Right, avoiding is intentional I just forgot to mention it in the
changelog. The bio has no data attached so integrity checker will skip
it.
> > /*
> > @@ -3522,9 +3513,10 @@ static void write_dev_flush(struct btrfs_device *device)
> > static int wait_dev_flush(struct btrfs_device *device)
> > {
> > int ret = 0;
> > + struct request_queue *q = bdev_get_queue(device->bdev);
> > struct bio *bio = device->flush_bio;
> >
> > - if (!bio)
> > + if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags))
> > return 0;
>
> It returns here if its write through. Which can be toggled
> after write_dev_flush() has been called such as..
>
> echo "write back" > /sys/block/sdd/queue/write_cache
> write_dev_flush(sdd)
> echo "write through" > /sys/block/sdd/queue/write_cache
> wait_dev_flush(sdd)
>
> So it would fails to check error.
Yeah, the bio would stay in flight. I had to read more about the flushes
but I apparently mixed it up with FUA. Toggling write cache needs to be
handled properly which needs to pull the relevant bits from patch 4/5
and the force_dev_flush sysfs knob does not make sense, as you noted.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
