| Message ID | 20200130152218.7600-4-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | request_alignment vs file size | expand |
On 30.01.20 16:22, Vladimir Sementsov-Ogievskiy wrote: > Prior to the commit the following command lead to crash: > > ./qemu-io --image-opts -c 'write 0 512' \ > driver=blkdebug,align=4096,image.driver=null-co,image.size=512 > > It failes on assertion in bdrv_aligned_pwritev: > "end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE" > > The problem is obvious: 512 is aligned to 4096 and becomes larger than > file size. And the core bad thing is that file size is unaligned to > request_alignment. > > Let's catch such case on bdrv_open_driver and fail. I think we had a discussion on this before, but I can’t find it right now. (Although I think that had more to do with something in the file-posix driver, because it wasn’t limited to alignments above 512.) In any case, the file itself is totally valid. Most importantly, qcow2 will regularly create files with unaligned file lengths. So let me create a qcow2 image on a 4k-aligned device: $ truncate 512M fs.img $ sudo losetup -f --show -b 4096 fs.img /dev/loop0 $ sudo mkfs.ext4 /dev/loop0 [...] $ sudo mount /dev/loop0 /mnt/tmp $ sudo ./qemu-img create -f qcow2 /mnt/tmp/foo.qcow2 64M Formatting '/mnt/tmp/foo.qcow2', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16 $ sudo ./qemu-io -t none -c quit /mnt/tmp/foo.qcow2 qemu-io: can't open device /mnt/tmp/foo.qcow2: File size is unaligned to request alignment Which is too bad. So the real solution would probably... Be to align the file size up to the alignment? Max > Note, that file size and request_alignment may become out of sync > later, so this commit is not full fix of the problem, but it's better > than nothing. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/block.c b/block.c > index ecd09dbbfd..4cfc6c33a2 100644 > --- a/block.c > +++ b/block.c > @@ -1324,6 +1324,13 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, > assert(bdrv_min_mem_align(bs) != 0); > assert(is_power_of_2(bs->bl.request_alignment)); > > + if (bs->bl.request_alignment > 512 && > + !QEMU_IS_ALIGNED(bs->total_sectors, bs->bl.request_alignment / 512)) > + { > + error_setg(errp, "File size is unaligned to request alignment"); > + return -EINVAL; > + } > + > for (i = 0; i < bs->quiesce_counter; i++) { > if (drv->bdrv_co_drain_begin) { > drv->bdrv_co_drain_begin(bs); >
On 3/11/20 6:06 AM, Max Reitz wrote: > On 30.01.20 16:22, Vladimir Sementsov-Ogievskiy wrote: >> Prior to the commit the following command lead to crash: >> >> ./qemu-io --image-opts -c 'write 0 512' \ >> driver=blkdebug,align=4096,image.driver=null-co,image.size=512 >> >> It failes on assertion in bdrv_aligned_pwritev: >> "end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE" >> >> The problem is obvious: 512 is aligned to 4096 and becomes larger than >> file size. And the core bad thing is that file size is unaligned to >> request_alignment. >> >> Let's catch such case on bdrv_open_driver and fail. > > I think we had a discussion on this before, but I can’t find it right > now. (Although I think that had more to do with something in the > file-posix driver, because it wasn’t limited to alignments above 512.) > > In any case, the file itself is totally valid. Most importantly, qcow2 > will regularly create files with unaligned file lengths. > > So let me create a qcow2 image on a 4k-aligned device: > > $ truncate 512M fs.img > $ sudo losetup -f --show -b 4096 fs.img > /dev/loop0 > $ sudo mkfs.ext4 /dev/loop0 > [...] > $ sudo mount /dev/loop0 /mnt/tmp > > $ sudo ./qemu-img create -f qcow2 /mnt/tmp/foo.qcow2 64M > Formatting '/mnt/tmp/foo.qcow2', fmt=qcow2 size=67108864 > cluster_size=65536 lazy_refcounts=off refcount_bits=16 > $ sudo ./qemu-io -t none -c quit /mnt/tmp/foo.qcow2 > qemu-io: can't open device /mnt/tmp/foo.qcow2: File size is unaligned to > request alignment > > Which is too bad. > > So the real solution would probably... Be to align the file size up to > the alignment? Or to bite the bullet and finally implement byte-accurate size everywhere (instead of our current insistence on rounding size up to 512-byte multiples). If we have to deal with unaligned tails anyways, it's better to make the code universally applicable whether that unaligned tail is to 512 or to 4k, than to have it work for 512 but to fail for 4k.
11.03.2020 15:29, Eric Blake wrote: > On 3/11/20 6:06 AM, Max Reitz wrote: >> On 30.01.20 16:22, Vladimir Sementsov-Ogievskiy wrote: >>> Prior to the commit the following command lead to crash: >>> >>> ./qemu-io --image-opts -c 'write 0 512' \ >>> driver=blkdebug,align=4096,image.driver=null-co,image.size=512 >>> >>> It failes on assertion in bdrv_aligned_pwritev: >>> "end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE" >>> >>> The problem is obvious: 512 is aligned to 4096 and becomes larger than >>> file size. And the core bad thing is that file size is unaligned to >>> request_alignment. >>> >>> Let's catch such case on bdrv_open_driver and fail. >> >> I think we had a discussion on this before, but I can’t find it right >> now. (Although I think that had more to do with something in the >> file-posix driver, because it wasn’t limited to alignments above 512.) >> >> In any case, the file itself is totally valid. Most importantly, qcow2 >> will regularly create files with unaligned file lengths. >> >> So let me create a qcow2 image on a 4k-aligned device: >> >> $ truncate 512M fs.img >> $ sudo losetup -f --show -b 4096 fs.img >> /dev/loop0 >> $ sudo mkfs.ext4 /dev/loop0 >> [...] >> $ sudo mount /dev/loop0 /mnt/tmp >> >> $ sudo ./qemu-img create -f qcow2 /mnt/tmp/foo.qcow2 64M >> Formatting '/mnt/tmp/foo.qcow2', fmt=qcow2 size=67108864 >> cluster_size=65536 lazy_refcounts=off refcount_bits=16 >> $ sudo ./qemu-io -t none -c quit /mnt/tmp/foo.qcow2 >> qemu-io: can't open device /mnt/tmp/foo.qcow2: File size is unaligned to >> request alignment >> >> Which is too bad. >> >> So the real solution would probably... Be to align the file size up to >> the alignment? > > Or to bite the bullet and finally implement byte-accurate size everywhere (instead of our current insistence on rounding size up to 512-byte multiples). If we have to deal with unaligned tails anyways, it's better to make the code universally applicable whether that unaligned tail is to 512 or to 4k, than to have it work for 512 but to fail for 4k. > But how it helps with the problem of files unaligned to request_alignment defined by driver?
On 3/12/20 4:06 AM, Vladimir Sementsov-Ogievskiy wrote: >>> So the real solution would probably... Be to align the file size up to >>> the alignment? >> >> Or to bite the bullet and finally implement byte-accurate size >> everywhere (instead of our current insistence on rounding size up to >> 512-byte multiples). If we have to deal with unaligned tails anyways, >> it's better to make the code universally applicable whether that >> unaligned tail is to 512 or to 4k, than to have it work for 512 but to >> fail for 4k. >> > > But how it helps with the problem of files unaligned to > request_alignment defined by driver? In a byte-accurate world, no driver should ever report an unaligned size. If the driver is capable of sub-sector sizes, it is also capable of sub-sector I/O and should state as such in its request_alignment. The block layer then takes care of ensuring that any access of the final unaligned sector or 4k region either leaves the bytes past EOF alone, or at most reads those bytes as zeroes, and maybe permits attempts to write no-op zeroes to those bytes, but gracefully forbids attempts to store data that would cause the file to be resized. But that's the ideal world, and it requires a lot of code auditing to get there. It probably won't happen in time for the 5.0 release.
11.03.2020 14:06, Max Reitz wrote: > On 30.01.20 16:22, Vladimir Sementsov-Ogievskiy wrote: >> Prior to the commit the following command lead to crash: >> >> ./qemu-io --image-opts -c 'write 0 512' \ >> driver=blkdebug,align=4096,image.driver=null-co,image.size=512 >> >> It failes on assertion in bdrv_aligned_pwritev: >> "end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE" >> >> The problem is obvious: 512 is aligned to 4096 and becomes larger than >> file size. And the core bad thing is that file size is unaligned to >> request_alignment. >> >> Let's catch such case on bdrv_open_driver and fail. > > I think we had a discussion on this before, but I can’t find it right > now. (Although I think that had more to do with something in the > file-posix driver, because it wasn’t limited to alignments above 512.) > > In any case, the file itself is totally valid. Most importantly, qcow2 > will regularly create files with unaligned file lengths. > > So let me create a qcow2 image on a 4k-aligned device: > > $ truncate 512M fs.img > $ sudo losetup -f --show -b 4096 fs.img > /dev/loop0 > $ sudo mkfs.ext4 /dev/loop0 > [...] > $ sudo mount /dev/loop0 /mnt/tmp > > $ sudo ./qemu-img create -f qcow2 /mnt/tmp/foo.qcow2 64M > Formatting '/mnt/tmp/foo.qcow2', fmt=qcow2 size=67108864 > cluster_size=65536 lazy_refcounts=off refcount_bits=16 > $ sudo ./qemu-io -t none -c quit /mnt/tmp/foo.qcow2 > qemu-io: can't open device /mnt/tmp/foo.qcow2: File size is unaligned to > request alignment > > Which is too bad. What exactly is bad? Is it correct that create succeeded? Without new error, how would qcow2 driver read from unaligned tail of file-posix? It will crash, isn't it? > > So the real solution would probably... Be to align the file size up to > the alignment? On creation, you mean? > > Max > >> Note, that file size and request_alignment may become out of sync >> later, so this commit is not full fix of the problem, but it's better >> than nothing. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/block.c b/block.c >> index ecd09dbbfd..4cfc6c33a2 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1324,6 +1324,13 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, >> assert(bdrv_min_mem_align(bs) != 0); >> assert(is_power_of_2(bs->bl.request_alignment)); >> >> + if (bs->bl.request_alignment > 512 && >> + !QEMU_IS_ALIGNED(bs->total_sectors, bs->bl.request_alignment / 512)) >> + { >> + error_setg(errp, "File size is unaligned to request alignment"); >> + return -EINVAL; >> + } >> + >> for (i = 0; i < bs->quiesce_counter; i++) { >> if (drv->bdrv_co_drain_begin) { >> drv->bdrv_co_drain_begin(bs); >> > >
12.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote: > 11.03.2020 14:06, Max Reitz wrote: >> On 30.01.20 16:22, Vladimir Sementsov-Ogievskiy wrote: >>> Prior to the commit the following command lead to crash: >>> >>> ./qemu-io --image-opts -c 'write 0 512' \ >>> driver=blkdebug,align=4096,image.driver=null-co,image.size=512 >>> >>> It failes on assertion in bdrv_aligned_pwritev: >>> "end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE" >>> >>> The problem is obvious: 512 is aligned to 4096 and becomes larger than >>> file size. And the core bad thing is that file size is unaligned to >>> request_alignment. >>> >>> Let's catch such case on bdrv_open_driver and fail. >> >> I think we had a discussion on this before, but I can’t find it right >> now. (Although I think that had more to do with something in the >> file-posix driver, because it wasn’t limited to alignments above 512.) >> >> In any case, the file itself is totally valid. Most importantly, qcow2 >> will regularly create files with unaligned file lengths. >> >> So let me create a qcow2 image on a 4k-aligned device: >> >> $ truncate 512M fs.img >> $ sudo losetup -f --show -b 4096 fs.img >> /dev/loop0 >> $ sudo mkfs.ext4 /dev/loop0 >> [...] >> $ sudo mount /dev/loop0 /mnt/tmp >> >> $ sudo ./qemu-img create -f qcow2 /mnt/tmp/foo.qcow2 64M >> Formatting '/mnt/tmp/foo.qcow2', fmt=qcow2 size=67108864 >> cluster_size=65536 lazy_refcounts=off refcount_bits=16 >> $ sudo ./qemu-io -t none -c quit /mnt/tmp/foo.qcow2 >> qemu-io: can't open device /mnt/tmp/foo.qcow2: File size is unaligned to >> request alignment >> >> Which is too bad. > > What exactly is bad? > > Is it correct that create succeeded? Without new error, how would qcow2 driver > read from unaligned tail of file-posix? It will crash, isn't it? Hmm, it crashes only on write-part if don't have RESIZE permission.. So for qcow2 everything is OK. And generic read don't care about reading past-EOF because of alignment, read is passed to driver. > >> >> So the real solution would probably... Be to align the file size up to >> the alignment? > > On creation, you mean? > >> >> Max >> >>> Note, that file size and request_alignment may become out of sync >>> later, so this commit is not full fix of the problem, but it's better >>> than nothing. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> block.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/block.c b/block.c >>> index ecd09dbbfd..4cfc6c33a2 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -1324,6 +1324,13 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, >>> assert(bdrv_min_mem_align(bs) != 0); >>> assert(is_power_of_2(bs->bl.request_alignment)); >>> + if (bs->bl.request_alignment > 512 && >>> + !QEMU_IS_ALIGNED(bs->total_sectors, bs->bl.request_alignment / 512)) >>> + { >>> + error_setg(errp, "File size is unaligned to request alignment"); >>> + return -EINVAL; >>> + } >>> + >>> for (i = 0; i < bs->quiesce_counter; i++) { >>> if (drv->bdrv_co_drain_begin) { >>> drv->bdrv_co_drain_begin(bs); >>> >> >> > >
On 12.03.20 13:06, Vladimir Sementsov-Ogievskiy wrote: > 12.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote: >> 11.03.2020 14:06, Max Reitz wrote: >>> On 30.01.20 16:22, Vladimir Sementsov-Ogievskiy wrote: >>>> Prior to the commit the following command lead to crash: >>>> >>>> ./qemu-io --image-opts -c 'write 0 512' \ >>>> driver=blkdebug,align=4096,image.driver=null-co,image.size=512 >>>> >>>> It failes on assertion in bdrv_aligned_pwritev: >>>> "end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE" >>>> >>>> The problem is obvious: 512 is aligned to 4096 and becomes larger than >>>> file size. And the core bad thing is that file size is unaligned to >>>> request_alignment. >>>> >>>> Let's catch such case on bdrv_open_driver and fail. >>> >>> I think we had a discussion on this before, but I can’t find it right >>> now. (Although I think that had more to do with something in the >>> file-posix driver, because it wasn’t limited to alignments above 512.) >>> >>> In any case, the file itself is totally valid. Most importantly, qcow2 >>> will regularly create files with unaligned file lengths. >>> >>> So let me create a qcow2 image on a 4k-aligned device: >>> >>> $ truncate 512M fs.img Oops, “truncate --size=512M fs.img”, of course. >>> $ sudo losetup -f --show -b 4096 fs.img >>> /dev/loop0 >>> $ sudo mkfs.ext4 /dev/loop0 >>> [...] >>> $ sudo mount /dev/loop0 /mnt/tmp >>> >>> $ sudo ./qemu-img create -f qcow2 /mnt/tmp/foo.qcow2 64M >>> Formatting '/mnt/tmp/foo.qcow2', fmt=qcow2 size=67108864 >>> cluster_size=65536 lazy_refcounts=off refcount_bits=16 >>> $ sudo ./qemu-io -t none -c quit /mnt/tmp/foo.qcow2 >>> qemu-io: can't open device /mnt/tmp/foo.qcow2: File size is unaligned to >>> request alignment >>> >>> Which is too bad. >> >> What exactly is bad? That with this patch you can no longer create a qcow2 image on a 4k block size medium and then open it with O_DIRECT. >> Is it correct that create succeeded? Without new error, how would >> qcow2 driver >> read from unaligned tail of file-posix? It will crash, isn't it? Well, the above test works, which is better already. Basically part of the problem seems to be changing from unsafe caching (during create) to O_DIRECT. If we used O_DIRECT all the time, the image would always be aligned to the block alignment. Now that makes me wonder how such cases are handled right now. Do we crash? So I tried to create an image with 512 byte clusters, write to it with normal WB caching, thus creating an image that isn’t aligned to 4k: $ sudo ./qemu-img create -f qcow2 -o cluster_size=512 \ /mnt/tmp/foo.qcow2 64M [...] $ sudo ./qemu-io -c 'write -P 42 0 512' /mnt/tmp/foo.qcow2 [...] $ printf '%x\n' $(stat -c '%s' /mnt/tmp/foo.qcow2) 4a00 (So not aligned to 4k) $ sudo ./qemu-io -t none -c 'read -P 42 0 512' /mnt/tmp/foo.qcow2 read 512/512 bytes at offset 0 512 bytes, 1 ops; 00.00 sec (4.567 MiB/sec and 9352.6997 ops/sec) So actually, that works just fine right now. When I let file-posix print all pread() calls, this is what I can see: [...] pread(9, buf_ofs=+0, file_ofs=0x4000, size=0x1000) -> 2560 pread(9, buf_ofs=+0xa00, file_ofs=0x4a00, size=0x600) -> 0 [...] So all reads are increased to the alignment (4k), and to read the data cluster, we read all 4k from 0x4000 (even though it actually is just 512 bytes at 0x4800). This succeeds, even though just as a partial read (2560 == 0xa00), so we try to read the rest of the 4k block, but that doesn’t work (it’s at the EOF), and so we stop reading. handle_aiocb_rw() then zeroes out the rest of the buffer (below the “out” label) and everyone’s happy. So to me it looks like it works perfectly fine right now. No crashes. > Hmm, it crashes only on write-part if don't have RESIZE permission.. So > for qcow2 > everything is OK. > > And generic read don't care about reading past-EOF because of alignment, > read is passed > to driver. (And only after the test, I realize that you apparently answered the question yourself... Oops.) >>> So the real solution would probably... Be to align the file size up to >>> the alignment? >> >> On creation, you mean? Yes. But I suspect we’d need to do it all the time, because see my above example: The user might access an image with WB caching at one point, and then with O_DIRECT the next time. But I can see two problems: First, we don’t even know what the block alignment is. It’s hard enough to figure it out for O_DIRECT images, I’m not sure we can reliably do so for WB-cached images. Second, this wouldn’t help with pre-existing images. And note that “pre-existing” might mean an image a user creates on an FS with 512 byte blocks and then moves it to another with 4k blocks. So I don’t think aligning the file size up would work, actually. Except if we decided to always align it up to 4k, but then we’d still have a problem with older pre-existing images... Max >>> >>> Max >>> >>>> Note, that file size and request_alignment may become out of sync >>>> later, so this commit is not full fix of the problem, but it's better >>>> than nothing. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> block.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/block.c b/block.c >>>> index ecd09dbbfd..4cfc6c33a2 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -1324,6 +1324,13 @@ static int bdrv_open_driver(BlockDriverState >>>> *bs, BlockDriver *drv, >>>> assert(bdrv_min_mem_align(bs) != 0); >>>> assert(is_power_of_2(bs->bl.request_alignment)); >>>> + if (bs->bl.request_alignment > 512 && >>>> + !QEMU_IS_ALIGNED(bs->total_sectors, >>>> bs->bl.request_alignment / 512)) >>>> + { >>>> + error_setg(errp, "File size is unaligned to request >>>> alignment"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> for (i = 0; i < bs->quiesce_counter; i++) { >>>> if (drv->bdrv_co_drain_begin) { >>>> drv->bdrv_co_drain_begin(bs); >>>> >>> >>> >> >> > >
diff --git a/block.c b/block.c index ecd09dbbfd..4cfc6c33a2 100644 --- a/block.c +++ b/block.c @@ -1324,6 +1324,13 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, assert(bdrv_min_mem_align(bs) != 0); assert(is_power_of_2(bs->bl.request_alignment)); + if (bs->bl.request_alignment > 512 && + !QEMU_IS_ALIGNED(bs->total_sectors, bs->bl.request_alignment / 512)) + { + error_setg(errp, "File size is unaligned to request alignment"); + return -EINVAL; + } + for (i = 0; i < bs->quiesce_counter; i++) { if (drv->bdrv_co_drain_begin) { drv->bdrv_co_drain_begin(bs);
Prior to the commit the following command lead to crash: ./qemu-io --image-opts -c 'write 0 512' \ driver=blkdebug,align=4096,image.driver=null-co,image.size=512 It failes on assertion in bdrv_aligned_pwritev: "end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE" The problem is obvious: 512 is aligned to 4096 and becomes larger than file size. And the core bad thing is that file size is unaligned to request_alignment. Let's catch such case on bdrv_open_driver and fail. Note, that file size and request_alignment may become out of sync later, so this commit is not full fix of the problem, but it's better than nothing. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block.c | 7 +++++++ 1 file changed, 7 insertions(+)