| Message ID | 1458325289-17848-14-git-send-email-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18.03.2016 19:21, Kevin Wolf wrote: > This replaces the existing hack in the iscsi driver that sent the FUA > bit in writethrough mode and ignored the following flush in order to > optimise the number of roundtrips (see commit 73b5394e). > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/iscsi.c | 24 +++++++----------------- > 1 file changed, 7 insertions(+), 17 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
On 18.03.2016 19:21, Kevin Wolf wrote: > This replaces the existing hack in the iscsi driver that sent the FUA > bit in writethrough mode and ignored the following flush in order to > optimise the number of roundtrips (see commit 73b5394e). > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/iscsi.c | 24 +++++++----------------- > 1 file changed, 7 insertions(+), 17 deletions(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index 3b54536..4f75204 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c [...] > @@ -1851,7 +1840,8 @@ static BlockDriver bdrv_iscsi = { > .bdrv_co_discard = iscsi_co_discard, > .bdrv_co_write_zeroes = iscsi_co_write_zeroes, > .bdrv_co_readv = iscsi_co_readv, > - .bdrv_co_writev = iscsi_co_writev, > + .bdrv_co_writev_flags = iscsi_co_writev_flags, > + .supported_write_flags = BDRV_REQ_FUA, > .bdrv_co_flush_to_disk = iscsi_co_flush, > > #ifdef __linux__ > Hm, wait, maybe not R-b. I can see three places in block/io.c which call bdrv_co_writev(), and only one of them diverts to bdrv_co_writev_flags() if that is available. Maybe we don't need to care about the bounce-buffer case for write_zeroes, but I do think we need to care about the COR case. Of course bdrv_co_writev() can trivially be forwarded to bdrv_co_writev_flags(), but I'm not sure who is supposed to do this forwarding. I can imagine three ways: (1) Keep a wrapper per block driver. Simple, but not so elegant. (2) Make all bdrv_co_writev() callers call bdrv_co_writev_flags() if the former is not available but the latter is. (3) Introduce a generic function replacing every drv->bdrv_co_writev() call which then decides which driver function to invoke. Max
Am 26.03.2016 um 21:44 hat Max Reitz geschrieben: > On 18.03.2016 19:21, Kevin Wolf wrote: > > This replaces the existing hack in the iscsi driver that sent the FUA > > bit in writethrough mode and ignored the following flush in order to > > optimise the number of roundtrips (see commit 73b5394e). > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/iscsi.c | 24 +++++++----------------- > > 1 file changed, 7 insertions(+), 17 deletions(-) > > > > diff --git a/block/iscsi.c b/block/iscsi.c > > index 3b54536..4f75204 100644 > > --- a/block/iscsi.c > > +++ b/block/iscsi.c > > [...] > > > @@ -1851,7 +1840,8 @@ static BlockDriver bdrv_iscsi = { > > .bdrv_co_discard = iscsi_co_discard, > > .bdrv_co_write_zeroes = iscsi_co_write_zeroes, > > .bdrv_co_readv = iscsi_co_readv, > > - .bdrv_co_writev = iscsi_co_writev, > > + .bdrv_co_writev_flags = iscsi_co_writev_flags, > > + .supported_write_flags = BDRV_REQ_FUA, > > .bdrv_co_flush_to_disk = iscsi_co_flush, > > > > #ifdef __linux__ > > > > Hm, wait, maybe not R-b. I can see three places in block/io.c which call > bdrv_co_writev(), and only one of them diverts to bdrv_co_writev_flags() > if that is available. Maybe we don't need to care about the > bounce-buffer case for write_zeroes, but I do think we need to care > about the COR case. > > Of course bdrv_co_writev() can trivially be forwarded to > bdrv_co_writev_flags(), but I'm not sure who is supposed to do this > forwarding. I can imagine three ways: > > (1) Keep a wrapper per block driver. Simple, but not so elegant. > (2) Make all bdrv_co_writev() callers call bdrv_co_writev_flags() if > the former is not available but the latter is. > (3) Introduce a generic function replacing every drv->bdrv_co_writev() > call which then decides which driver function to invoke. Good catch, thanks! Going for (1) for now, because I think (2) is even less elegant and while I have a slight preference for (3) from the code perspective, it could be argued that it impacts the hot write path of raw images and I don't want to deal with potential performance changes that late in the cycle. And now that I'm writing this, I realise that the hot path already calls .bdrv_co_writev_flags, so that's not a real argument. But I've already implemented (1), so I'll leave it at that... The long term plan is anyway to convert everything to .bdrv_co_writev_flags. Kevin
diff --git a/block/iscsi.c b/block/iscsi.c index 3b54536..4f75204 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -70,7 +70,6 @@ typedef struct IscsiLun { bool lbprz; bool dpofua; bool has_write_same; - bool force_next_flush; bool request_timed_out; } IscsiLun; @@ -84,7 +83,6 @@ typedef struct IscsiTask { QEMUBH *bh; IscsiLun *iscsilun; QEMUTimer retry_timer; - bool force_next_flush; int err_code; } IscsiTask; @@ -282,8 +280,6 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status, } iTask->err_code = iscsi_translate_sense(&task->sense); error_report("iSCSI Failure: %s", iscsi_get_error(iscsi)); - } else { - iTask->iscsilun->force_next_flush |= iTask->force_next_flush; } out: @@ -452,15 +448,15 @@ static void iscsi_allocationmap_clear(IscsiLun *iscsilun, int64_t sector_num, } } -static int coroutine_fn iscsi_co_writev(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, - QEMUIOVector *iov) +static int coroutine_fn +iscsi_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors, + QEMUIOVector *iov, int flags) { IscsiLun *iscsilun = bs->opaque; struct IscsiTask iTask; uint64_t lba; uint32_t num_sectors; - int fua; + bool fua; if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { return -EINVAL; @@ -476,8 +472,7 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs, num_sectors = sector_qemu2lun(nb_sectors, iscsilun); iscsi_co_init_iscsitask(iscsilun, &iTask); retry: - fua = iscsilun->dpofua && !bdrv_enable_write_cache(bs); - iTask.force_next_flush = !fua; + fua = iscsilun->dpofua && (flags & BDRV_REQ_FUA); if (iscsilun->use_16_for_rw) { iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba, NULL, num_sectors * iscsilun->block_size, @@ -715,11 +710,6 @@ static int coroutine_fn iscsi_co_flush(BlockDriverState *bs) IscsiLun *iscsilun = bs->opaque; struct IscsiTask iTask; - if (!iscsilun->force_next_flush) { - return 0; - } - iscsilun->force_next_flush = false; - iscsi_co_init_iscsitask(iscsilun, &iTask); retry: if (iscsi_synchronizecache10_task(iscsilun->iscsi, iscsilun->lun, 0, 0, 0, @@ -1019,7 +1009,6 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, } iscsi_co_init_iscsitask(iscsilun, &iTask); - iTask.force_next_flush = true; retry: if (use_16_for_ws) { iTask.task = iscsi_writesame16_task(iscsilun->iscsi, iscsilun->lun, lba, @@ -1851,7 +1840,8 @@ static BlockDriver bdrv_iscsi = { .bdrv_co_discard = iscsi_co_discard, .bdrv_co_write_zeroes = iscsi_co_write_zeroes, .bdrv_co_readv = iscsi_co_readv, - .bdrv_co_writev = iscsi_co_writev, + .bdrv_co_writev_flags = iscsi_co_writev_flags, + .supported_write_flags = BDRV_REQ_FUA, .bdrv_co_flush_to_disk = iscsi_co_flush, #ifdef __linux__
This replaces the existing hack in the iscsi driver that sent the FUA bit in writethrough mode and ignored the following flush in order to optimise the number of roundtrips (see commit 73b5394e). Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/iscsi.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-)