| Message ID | 1516297747-107232-2-git-send-email-anton.nefedov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018-01-18 18:48, Anton Nefedov wrote: > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > --- > block/mirror.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/block/mirror.c b/block/mirror.c > index c9badc1..d18ec65 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1064,6 +1064,11 @@ static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts) > bdrv_refresh_filename(bs->backing->bs); > pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), > bs->backing->bs->filename); > + bs->supported_write_flags = BDRV_REQ_FUA & > + bs->backing->bs->supported_write_flags; > + bs->supported_zero_flags = > + (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) & > + bs->backing->bs->supported_zero_flags; > } > > static void bdrv_mirror_top_close(BlockDriverState *bs) Fundamentally OK, but why is this in *_refresh_filename()? Max
On 01/29/2018 01:21 PM, Max Reitz wrote: > On 2018-01-18 18:48, Anton Nefedov wrote: >> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Reviewed-by: Alberto Garcia <berto@igalia.com> >> --- >> block/mirror.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/block/mirror.c b/block/mirror.c >> index c9badc1..d18ec65 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -1064,6 +1064,11 @@ static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts) >> bdrv_refresh_filename(bs->backing->bs); >> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), >> bs->backing->bs->filename); >> + bs->supported_write_flags = BDRV_REQ_FUA & >> + bs->backing->bs->supported_write_flags; >> + bs->supported_zero_flags = >> + (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) & >> + bs->backing->bs->supported_zero_flags; >> } >> >> static void bdrv_mirror_top_close(BlockDriverState *bs) > > Fundamentally OK, but why is this in *_refresh_filename()? Indeed, I missed that (or maybe it got moved during a botched rebase?). For comparison, blkdebug sets it during blkdebug_open(), and nbd sets it during nbd_client_init() (called during nbd_open()).
On 29/1/2018 10:26 PM, Eric Blake wrote: > On 01/29/2018 01:21 PM, Max Reitz wrote: >> On 2018-01-18 18:48, Anton Nefedov wrote: >>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> Reviewed-by: Alberto Garcia <berto@igalia.com> >>> --- >>> block/mirror.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/block/mirror.c b/block/mirror.c >>> index c9badc1..d18ec65 100644 >>> --- a/block/mirror.c >>> +++ b/block/mirror.c >>> @@ -1064,6 +1064,11 @@ static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts) >>> bdrv_refresh_filename(bs->backing->bs); >>> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), >>> bs->backing->bs->filename); >>> + bs->supported_write_flags = BDRV_REQ_FUA & >>> + bs->backing->bs->supported_write_flags; >>> + bs->supported_zero_flags = >>> + (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) & >>> + bs->backing->bs->supported_zero_flags; >>> } >>> >>> static void bdrv_mirror_top_close(BlockDriverState *bs) >> >> Fundamentally OK, but why is this in *_refresh_filename()? > > Indeed, I missed that (or maybe it got moved during a botched rebase?). > For comparison, blkdebug sets it during blkdebug_open(), and nbd sets it > during nbd_client_init() (called during nbd_open()). > We need a backing bs here and I believe it's not generally set at the time of .bdrv_open()
On 01/30/2018 06:15 AM, Anton Nefedov wrote: >>>> @@ -1064,6 +1064,11 @@ static void >>>> bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts) >>>> bdrv_refresh_filename(bs->backing->bs); >>>> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), >>>> bs->backing->bs->filename); >>>> + bs->supported_write_flags = BDRV_REQ_FUA & >>>> + bs->backing->bs->supported_write_flags; >>> Fundamentally OK, but why is this in *_refresh_filename()? >> >> Indeed, I missed that (or maybe it got moved during a botched rebase?). >> For comparison, blkdebug sets it during blkdebug_open(), and nbd sets it >> during nbd_client_init() (called during nbd_open()). >> > > We need a backing bs here and I believe it's not generally set at the > time of .bdrv_open() Then is mirror_start_job() a better location, right after we call bdrv_new_open_driver()? (Maybe this just goes to show I haven't fully traced the lifecycle of the mirror driver, and it may all be changing anyways as we try to fix the BDS graph modifications related with mirrors).
On 2018-01-30 13:15, Anton Nefedov wrote: > > > On 29/1/2018 10:26 PM, Eric Blake wrote: >> On 01/29/2018 01:21 PM, Max Reitz wrote: >>> On 2018-01-18 18:48, Anton Nefedov wrote: >>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> >>>> Reviewed-by: Alberto Garcia <berto@igalia.com> >>>> --- >>>> block/mirror.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/block/mirror.c b/block/mirror.c >>>> index c9badc1..d18ec65 100644 >>>> --- a/block/mirror.c >>>> +++ b/block/mirror.c >>>> @@ -1064,6 +1064,11 @@ static void >>>> bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts) >>>> bdrv_refresh_filename(bs->backing->bs); >>>> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), >>>> bs->backing->bs->filename); >>>> + bs->supported_write_flags = BDRV_REQ_FUA & >>>> + bs->backing->bs->supported_write_flags; >>>> + bs->supported_zero_flags = >>>> + (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) & >>>> + bs->backing->bs->supported_zero_flags; >>>> } >>>> static void bdrv_mirror_top_close(BlockDriverState *bs) >>> >>> Fundamentally OK, but why is this in *_refresh_filename()? >> >> Indeed, I missed that (or maybe it got moved during a botched rebase?). >> For comparison, blkdebug sets it during blkdebug_open(), and nbd sets it >> during nbd_client_init() (called during nbd_open()). >> > > We need a backing bs here and I believe it's not generally set at the > time of .bdrv_open(). Well, but *_refresh_filename() is just wrong. This driver doesn't have .bdrv_open() at all, and we create it fully manually in mirror_start_job() anyway (as Eric has said). Therefore, we can just do this right after bdrv_append() there has succeeded. Max
diff --git a/block/mirror.c b/block/mirror.c index c9badc1..d18ec65 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1064,6 +1064,11 @@ static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts) bdrv_refresh_filename(bs->backing->bs); pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->backing->bs->filename); + bs->supported_write_flags = BDRV_REQ_FUA & + bs->backing->bs->supported_write_flags; + bs->supported_zero_flags = + (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) & + bs->backing->bs->supported_zero_flags; } static void bdrv_mirror_top_close(BlockDriverState *bs)