| Message ID | 20190323212639.579-2-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | NBD block status failures | expand |
Am 23.03.2019 um 22:26 hat Eric Blake geschrieben: > If bdrv_block_status_above() fails, we are aborting the convert > process but failing to print an error message. Broken in commit > 690c7301 (v2.4) when rewriting convert's logic. > > Discovered when teaching nbdkit to support NBD_CMD_BLOCK_STATUS, and > accidentally violating the protocol by returning more than one extent > in spite of qemu asking for NBD_CMD_FLAG_REQ_ONE. The qemu NBD code > should probably handle the server's non-compliance more gracefully > than failing with EINVAL, but qemu-img shouldn't be silently > squelching any block status failures. It doesn't help that qemu 3.1 > masks the qemu-img bug with extra noise that the nbd code is dumping > to stderr (that noise was cleaned up in d8b4bad8). > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
24.03.2019 0:26, Eric Blake wrote: > If bdrv_block_status_above() fails, we are aborting the convert > process but failing to print an error message. Broken in commit > 690c7301 (v2.4) when rewriting convert's logic. > > Discovered when teaching nbdkit to support NBD_CMD_BLOCK_STATUS, and > accidentally violating the protocol by returning more than one extent > in spite of qemu asking for NBD_CMD_FLAG_REQ_ONE. The qemu NBD code > should probably handle the server's non-compliance more gracefully > than failing with EINVAL, but qemu-img shouldn't be silently > squelching any block status failures. It doesn't help that qemu 3.1 > masks the qemu-img bug with extra noise that the nbd code is dumping > to stderr (that noise was cleaned up in d8b4bad8). > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > qemu-img.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/qemu-img.c b/qemu-img.c > index 5fac8407428..a67c28e9ae5 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1630,6 +1630,8 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) > count, &count, NULL, NULL); > } > if (ret < 0) { > + error_report("Could not read sector %" PRId64 " metadata: %s", > + sector_num, strerror(-ret)); hmm first time I see that is called "metadata", more common pattern is just s/ metadata:/:/ > return ret; > } > n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE); >
25.03.2019 13:27, Vladimir Sementsov-Ogievskiy wrote: > 24.03.2019 0:26, Eric Blake wrote: >> If bdrv_block_status_above() fails, we are aborting the convert >> process but failing to print an error message. Broken in commit >> 690c7301 (v2.4) when rewriting convert's logic. >> >> Discovered when teaching nbdkit to support NBD_CMD_BLOCK_STATUS, and >> accidentally violating the protocol by returning more than one extent >> in spite of qemu asking for NBD_CMD_FLAG_REQ_ONE. The qemu NBD code >> should probably handle the server's non-compliance more gracefully >> than failing with EINVAL, but qemu-img shouldn't be silently >> squelching any block status failures. It doesn't help that qemu 3.1 >> masks the qemu-img bug with extra noise that the nbd code is dumping >> to stderr (that noise was cleaned up in d8b4bad8). >> >> Reported-by: Richard W.M. Jones <rjones@redhat.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > >> --- >> qemu-img.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 5fac8407428..a67c28e9ae5 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1630,6 +1630,8 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) >> count, &count, NULL, NULL); >> } >> if (ret < 0) { >> + error_report("Could not read sector %" PRId64 " metadata: %s", >> + sector_num, strerror(-ret)); > > hmm first time I see that is called "metadata", more common pattern is just s/ metadata:/:/ Oops, it's OK, "metadata" is about "sector". > >> return ret; >> } >> n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE); >> > >
Am 25.03.2019 um 11:27 hat Vladimir Sementsov-Ogievskiy geschrieben: > 24.03.2019 0:26, Eric Blake wrote: > > If bdrv_block_status_above() fails, we are aborting the convert > > process but failing to print an error message. Broken in commit > > 690c7301 (v2.4) when rewriting convert's logic. > > > > Discovered when teaching nbdkit to support NBD_CMD_BLOCK_STATUS, and > > accidentally violating the protocol by returning more than one extent > > in spite of qemu asking for NBD_CMD_FLAG_REQ_ONE. The qemu NBD code > > should probably handle the server's non-compliance more gracefully > > than failing with EINVAL, but qemu-img shouldn't be silently > > squelching any block status failures. It doesn't help that qemu 3.1 > > masks the qemu-img bug with extra noise that the nbd code is dumping > > to stderr (that noise was cleaned up in d8b4bad8). > > > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > > Signed-off-by: Eric Blake <eblake@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > > --- > > qemu-img.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/qemu-img.c b/qemu-img.c > > index 5fac8407428..a67c28e9ae5 100644 > > --- a/qemu-img.c > > +++ b/qemu-img.c > > @@ -1630,6 +1630,8 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) > > count, &count, NULL, NULL); > > } > > if (ret < 0) { > > + error_report("Could not read sector %" PRId64 " metadata: %s", > > + sector_num, strerror(-ret)); > > hmm first time I see that is called "metadata", more common pattern is > just s/ metadata:/:/ I think that would be misleading, because I would understand that actual data I/O (bdrv_co_preadv) has failed. Actually, before sending my R-b, for a moment I thought of suggesting to make the message something like "Could not get block status for sector ..." to make it less likely that someone just reads the start and misinterprets the message. But then I decided that the colour of this specific bike shed isn't important enough to me to request a respin. Kevin
On 3/25/19 6:53 AM, Kevin Wolf wrote: > Am 25.03.2019 um 11:27 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 24.03.2019 0:26, Eric Blake wrote: >>> If bdrv_block_status_above() fails, we are aborting the convert >>> process but failing to print an error message. Broken in commit >>> 690c7301 (v2.4) when rewriting convert's logic. >>> >>> Discovered when teaching nbdkit to support NBD_CMD_BLOCK_STATUS, and >>> accidentally violating the protocol by returning more than one extent >>> in spite of qemu asking for NBD_CMD_FLAG_REQ_ONE. The qemu NBD code >>> should probably handle the server's non-compliance more gracefully >>> than failing with EINVAL, but qemu-img shouldn't be silently >>> squelching any block status failures. It doesn't help that qemu 3.1 >>> masks the qemu-img bug with extra noise that the nbd code is dumping >>> to stderr (that noise was cleaned up in d8b4bad8). >>> @@ -1630,6 +1630,8 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) >>> count, &count, NULL, NULL); >>> } >>> if (ret < 0) { >>> + error_report("Could not read sector %" PRId64 " metadata: %s", >>> + sector_num, strerror(-ret)); >> >> hmm first time I see that is called "metadata", more common pattern is >> just s/ metadata:/:/ > > I think that would be misleading, because I would understand that actual > data I/O (bdrv_co_preadv) has failed. > > Actually, before sending my R-b, for a moment I thought of suggesting to > make the message something like "Could not get block status for sector > ..." to make it less likely that someone just reads the start and > misinterprets the message. But then I decided that the colour of this > specific bike shed isn't important enough to me to request a respin. Prior to commit 690c7301, it was "error while reading block status of sector % "PRId64 " %s" I can change that while queuing through my NBD tree. Thanks; applying to my queue for 4.0.
diff --git a/qemu-img.c b/qemu-img.c index 5fac8407428..a67c28e9ae5 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1630,6 +1630,8 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) count, &count, NULL, NULL); } if (ret < 0) { + error_report("Could not read sector %" PRId64 " metadata: %s", + sector_num, strerror(-ret)); return ret; } n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
If bdrv_block_status_above() fails, we are aborting the convert process but failing to print an error message. Broken in commit 690c7301 (v2.4) when rewriting convert's logic. Discovered when teaching nbdkit to support NBD_CMD_BLOCK_STATUS, and accidentally violating the protocol by returning more than one extent in spite of qemu asking for NBD_CMD_FLAG_REQ_ONE. The qemu NBD code should probably handle the server's non-compliance more gracefully than failing with EINVAL, but qemu-img shouldn't be silently squelching any block status failures. It doesn't help that qemu 3.1 masks the qemu-img bug with extra noise that the nbd code is dumping to stderr (that noise was cleaned up in d8b4bad8). Reported-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- qemu-img.c | 2 ++ 1 file changed, 2 insertions(+)