All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for-5.1 v2 0/2] qemu-img convert: Fix abort with unaligned image size
@ 2020-07-16 14:25 Kevin Wolf
  2020-07-16 14:26 ` [PATCH for-5.1 v2 1/2] block: Require aligned image size to avoid assertion failure Kevin Wolf
  2020-07-16 14:26 ` [PATCH for-5.1 v2 2/2] file-posix: Allow byte-aligned O_DIRECT with NFS Kevin Wolf
  0 siblings, 2 replies; 7+ messages in thread
From: Kevin Wolf @ 2020-07-16 14:25 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel, mreitz

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1834646

Patch 1 fixes the assertion failure by failing gracefully when opening
an image whose size isn't aligned to the required request alignment.

Patch 2 relaxes the restrictions for NFS, which actually supports byte
alignment, but incorrectly gets a 4k request alignment in the file-posix
block driver.

v2:
- Don't fail opening unaligned images, but requesting WRITE permission
  without RESIZE. This keeps qcow2 images with unaligned metadata at EOF
  working. [Max]

Kevin Wolf (2):
  block: Require aligned image size to avoid assertion failure
  file-posix: Allow byte-aligned O_DIRECT with NFS

 block.c            | 16 ++++++++++++++++
 block/file-posix.c | 26 +++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

-- 
2.25.4



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH for-5.1 v2 1/2] block: Require aligned image size to avoid assertion failure
  2020-07-16 14:25 [PATCH for-5.1 v2 0/2] qemu-img convert: Fix abort with unaligned image size Kevin Wolf
@ 2020-07-16 14:26 ` Kevin Wolf
  2020-07-17 11:02   ` Max Reitz
  2020-07-16 14:26 ` [PATCH for-5.1 v2 2/2] file-posix: Allow byte-aligned O_DIRECT with NFS Kevin Wolf
  1 sibling, 1 reply; 7+ messages in thread
From: Kevin Wolf @ 2020-07-16 14:26 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel, mreitz

Unaligned requests will automatically be aligned to bl.request_alignment
and we can't extend write requests to access space beyond the end of the
image without resizing the image, so if we have the WRITE permission,
but not the RESIZE one, it's required that the image size is aligned.

Failing to meet this requirement could cause assertion failures like
this if RESIZE permissions weren't requested:

qemu-img: block/io.c:1910: bdrv_co_write_req_prepare: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.

This was e.g. triggered by qemu-img converting to a target image with 4k
request alignment when the image was only aligned to 512 bytes, but not
to 4k.

Turn this into a graceful error in bdrv_check_perm() so that WRITE
without RESIZE can only be taken if the image size is aligned. If a user
holds both permissions and drops only RESIZE, the function will return
an error, but bdrv_child_try_set_perm() will ignore the failure silently
if permissions are only requested to be relaxed and just keep both
permissions while returning success.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/block.c b/block.c
index 35a372df57..6371928edb 100644
--- a/block.c
+++ b/block.c
@@ -2025,6 +2025,22 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
         return -EPERM;
     }
 
+    /*
+     * Unaligned requests will automatically be aligned to bl.request_alignment
+     * and without RESIZE we can't extend requests to write to space beyond the
+     * end of the image, so it's required that the image size is aligned.
+     */
+    if ((cumulative_perms & BLK_PERM_WRITE) &&
+        !(cumulative_perms & BLK_PERM_RESIZE))
+    {
+        if ((bs->total_sectors * BDRV_SECTOR_SIZE) % bs->bl.request_alignment) {
+            error_setg(errp, "Cannot get 'write' permission without 'resize': "
+                             "Image size is not a multiple of request "
+                             "alignment");
+            return -EPERM;
+        }
+    }
+
     /* Check this node */
     if (!drv) {
         return 0;
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH for-5.1 v2 2/2] file-posix: Allow byte-aligned O_DIRECT with NFS
  2020-07-16 14:25 [PATCH for-5.1 v2 0/2] qemu-img convert: Fix abort with unaligned image size Kevin Wolf
  2020-07-16 14:26 ` [PATCH for-5.1 v2 1/2] block: Require aligned image size to avoid assertion failure Kevin Wolf
@ 2020-07-16 14:26 ` Kevin Wolf
  2020-07-17 11:15   ` Max Reitz
  1 sibling, 1 reply; 7+ messages in thread
From: Kevin Wolf @ 2020-07-16 14:26 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel, mreitz

Since commit a6b257a08e3 ('file-posix: Handle undetectable alignment'),
we assume that if we open a file with O_DIRECT and alignment probing
returns 1, we just couldn't find out the real alignment requirement
because some filesystems make the requirement only for allocated blocks.
In this case, a safe default of 4k is used.

This is too strict for NFS, which does actually allow byte-aligned
requests even with O_DIRECT. Because we can't distinguish both cases
with generic code, let's just look at the file system magic and disable
s->needs_alignment for NFS. This way, O_DIRECT can still be used on NFS
for images that are not aligned to 4k.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/file-posix.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 8067e238cb..ae8190edab 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -62,10 +62,12 @@
 #include <sys/ioctl.h>
 #include <sys/param.h>
 #include <sys/syscall.h>
+#include <sys/vfs.h>
 #include <linux/cdrom.h>
 #include <linux/fd.h>
 #include <linux/fs.h>
 #include <linux/hdreg.h>
+#include <linux/magic.h>
 #include <scsi/sg.h>
 #ifdef __s390__
 #include <asm/dasd.h>
@@ -300,6 +302,28 @@ static int probe_physical_blocksize(int fd, unsigned int *blk_size)
 #endif
 }
 
+/*
+ * Returns true if no alignment restrictions are necessary even for files
+ * opened with O_DIRECT.
+ *
+ * raw_probe_alignment() probes the required alignment and assume that 1 means
+ * the probing failed, so it falls back to a safe default of 4k. This can be
+ * avoided if we know that byte alignment is okay for the file.
+ */
+static bool dio_byte_aligned(int fd)
+{
+#ifdef __linux__
+    struct statfs buf;
+    int ret;
+
+    ret = fstatfs(fd, &buf);
+    if (ret == 0 && buf.f_type == NFS_SUPER_MAGIC) {
+        return true;
+    }
+#endif
+    return false;
+}
+
 /* Check if read is allowed with given memory buffer and length.
  *
  * This function is used to check O_DIRECT memory buffer and request alignment.
@@ -629,7 +653,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
 
     s->has_discard = true;
     s->has_write_zeroes = true;
-    if ((bs->open_flags & BDRV_O_NOCACHE) != 0) {
+    if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) {
         s->needs_alignment = true;
     }
 
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH for-5.1 v2 1/2] block: Require aligned image size to avoid assertion failure
  2020-07-16 14:26 ` [PATCH for-5.1 v2 1/2] block: Require aligned image size to avoid assertion failure Kevin Wolf
@ 2020-07-17 11:02   ` Max Reitz
  2020-07-17 11:32     ` Kevin Wolf
  0 siblings, 1 reply; 7+ messages in thread
From: Max Reitz @ 2020-07-17 11:02 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: qemu-devel


[-- Attachment #1.1: Type: text/plain, Size: 2718 bytes --]

On 16.07.20 16:26, Kevin Wolf wrote:
> Unaligned requests will automatically be aligned to bl.request_alignment
> and we can't extend write requests to access space beyond the end of the
> image without resizing the image, so if we have the WRITE permission,
> but not the RESIZE one, it's required that the image size is aligned.
> 
> Failing to meet this requirement could cause assertion failures like
> this if RESIZE permissions weren't requested:
> 
> qemu-img: block/io.c:1910: bdrv_co_write_req_prepare: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.
> 
> This was e.g. triggered by qemu-img converting to a target image with 4k
> request alignment when the image was only aligned to 512 bytes, but not
> to 4k.
> 
> Turn this into a graceful error in bdrv_check_perm() so that WRITE
> without RESIZE can only be taken if the image size is aligned. If a user
> holds both permissions and drops only RESIZE, the function will return
> an error, but bdrv_child_try_set_perm() will ignore the failure silently
> if permissions are only requested to be relaxed and just keep both
> permissions while returning success.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 35a372df57..6371928edb 100644
> --- a/block.c
> +++ b/block.c
> @@ -2025,6 +2025,22 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
>          return -EPERM;
>      }
>  
> +    /*
> +     * Unaligned requests will automatically be aligned to bl.request_alignment
> +     * and without RESIZE we can't extend requests to write to space beyond the
> +     * end of the image, so it's required that the image size is aligned.
> +     */
> +    if ((cumulative_perms & BLK_PERM_WRITE) &&

What about WRITE_UNCHANGED?  I think this would only matter with nodes
that can have backing files (i.e., qcow2 in practice) because
WRITE_UNCHANGED is only used by COR and block jobs doing something with
a backing chain, so it shouldn’t matter in practice, but, well.

So, either way:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +        !(cumulative_perms & BLK_PERM_RESIZE))
> +    {
> +        if ((bs->total_sectors * BDRV_SECTOR_SIZE) % bs->bl.request_alignment) {
> +            error_setg(errp, "Cannot get 'write' permission without 'resize': "
> +                             "Image size is not a multiple of request "
> +                             "alignment");
> +            return -EPERM;
> +        }
> +    }
> +
>      /* Check this node */
>      if (!drv) {
>          return 0;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH for-5.1 v2 2/2] file-posix: Allow byte-aligned O_DIRECT with NFS
  2020-07-16 14:26 ` [PATCH for-5.1 v2 2/2] file-posix: Allow byte-aligned O_DIRECT with NFS Kevin Wolf
@ 2020-07-17 11:15   ` Max Reitz
  0 siblings, 0 replies; 7+ messages in thread
From: Max Reitz @ 2020-07-17 11:15 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: qemu-devel


[-- Attachment #1.1: Type: text/plain, Size: 978 bytes --]

On 16.07.20 16:26, Kevin Wolf wrote:
> Since commit a6b257a08e3 ('file-posix: Handle undetectable alignment'),
> we assume that if we open a file with O_DIRECT and alignment probing
> returns 1, we just couldn't find out the real alignment requirement
> because some filesystems make the requirement only for allocated blocks.
> In this case, a safe default of 4k is used.
> 
> This is too strict for NFS, which does actually allow byte-aligned
> requests even with O_DIRECT. Because we can't distinguish both cases
> with generic code, let's just look at the file system magic and disable
> s->needs_alignment for NFS. This way, O_DIRECT can still be used on NFS
> for images that are not aligned to 4k.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/file-posix.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH for-5.1 v2 1/2] block: Require aligned image size to avoid assertion failure
  2020-07-17 11:02   ` Max Reitz
@ 2020-07-17 11:32     ` Kevin Wolf
  2020-07-17 11:36       ` Max Reitz
  0 siblings, 1 reply; 7+ messages in thread
From: Kevin Wolf @ 2020-07-17 11:32 UTC (permalink / raw)
  To: Max Reitz; +Cc: qemu-devel, qemu-block

[-- Attachment #1: Type: text/plain, Size: 3112 bytes --]

Am 17.07.2020 um 13:02 hat Max Reitz geschrieben:
> On 16.07.20 16:26, Kevin Wolf wrote:
> > Unaligned requests will automatically be aligned to bl.request_alignment
> > and we can't extend write requests to access space beyond the end of the
> > image without resizing the image, so if we have the WRITE permission,
> > but not the RESIZE one, it's required that the image size is aligned.
> > 
> > Failing to meet this requirement could cause assertion failures like
> > this if RESIZE permissions weren't requested:
> > 
> > qemu-img: block/io.c:1910: bdrv_co_write_req_prepare: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.
> > 
> > This was e.g. triggered by qemu-img converting to a target image with 4k
> > request alignment when the image was only aligned to 512 bytes, but not
> > to 4k.
> > 
> > Turn this into a graceful error in bdrv_check_perm() so that WRITE
> > without RESIZE can only be taken if the image size is aligned. If a user
> > holds both permissions and drops only RESIZE, the function will return
> > an error, but bdrv_child_try_set_perm() will ignore the failure silently
> > if permissions are only requested to be relaxed and just keep both
> > permissions while returning success.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 35a372df57..6371928edb 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2025,6 +2025,22 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
> >          return -EPERM;
> >      }
> >  
> > +    /*
> > +     * Unaligned requests will automatically be aligned to bl.request_alignment
> > +     * and without RESIZE we can't extend requests to write to space beyond the
> > +     * end of the image, so it's required that the image size is aligned.
> > +     */
> > +    if ((cumulative_perms & BLK_PERM_WRITE) &&
> 
> What about WRITE_UNCHANGED?  I think this would only matter with nodes
> that can have backing files (i.e., qcow2 in practice) because
> WRITE_UNCHANGED is only used by COR and block jobs doing something with
> a backing chain, so it shouldn’t matter in practice, but, well.

So basically just replacing the line with this?

    if ((cumulative_perms & (BLK_PERM_WRITE | BDRV_PERM_WRITE_UNCHANGED)) &&

I can do that while applying if it is what you mean.

> So, either way:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks!

Kevin

> > +        !(cumulative_perms & BLK_PERM_RESIZE))
> > +    {
> > +        if ((bs->total_sectors * BDRV_SECTOR_SIZE) % bs->bl.request_alignment) {
> > +            error_setg(errp, "Cannot get 'write' permission without 'resize': "
> > +                             "Image size is not a multiple of request "
> > +                             "alignment");
> > +            return -EPERM;
> > +        }
> > +    }
> > +
> >      /* Check this node */
> >      if (!drv) {
> >          return 0;
> > 
> 
> 




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH for-5.1 v2 1/2] block: Require aligned image size to avoid assertion failure
  2020-07-17 11:32     ` Kevin Wolf
@ 2020-07-17 11:36       ` Max Reitz
  0 siblings, 0 replies; 7+ messages in thread
From: Max Reitz @ 2020-07-17 11:36 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel, qemu-block


[-- Attachment #1.1: Type: text/plain, Size: 3195 bytes --]

On 17.07.20 13:32, Kevin Wolf wrote:
> Am 17.07.2020 um 13:02 hat Max Reitz geschrieben:
>> On 16.07.20 16:26, Kevin Wolf wrote:
>>> Unaligned requests will automatically be aligned to bl.request_alignment
>>> and we can't extend write requests to access space beyond the end of the
>>> image without resizing the image, so if we have the WRITE permission,
>>> but not the RESIZE one, it's required that the image size is aligned.
>>>
>>> Failing to meet this requirement could cause assertion failures like
>>> this if RESIZE permissions weren't requested:
>>>
>>> qemu-img: block/io.c:1910: bdrv_co_write_req_prepare: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.
>>>
>>> This was e.g. triggered by qemu-img converting to a target image with 4k
>>> request alignment when the image was only aligned to 512 bytes, but not
>>> to 4k.
>>>
>>> Turn this into a graceful error in bdrv_check_perm() so that WRITE
>>> without RESIZE can only be taken if the image size is aligned. If a user
>>> holds both permissions and drops only RESIZE, the function will return
>>> an error, but bdrv_child_try_set_perm() will ignore the failure silently
>>> if permissions are only requested to be relaxed and just keep both
>>> permissions while returning success.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  block.c | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index 35a372df57..6371928edb 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -2025,6 +2025,22 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
>>>          return -EPERM;
>>>      }
>>>  
>>> +    /*
>>> +     * Unaligned requests will automatically be aligned to bl.request_alignment
>>> +     * and without RESIZE we can't extend requests to write to space beyond the
>>> +     * end of the image, so it's required that the image size is aligned.
>>> +     */
>>> +    if ((cumulative_perms & BLK_PERM_WRITE) &&
>>
>> What about WRITE_UNCHANGED?  I think this would only matter with nodes
>> that can have backing files (i.e., qcow2 in practice) because
>> WRITE_UNCHANGED is only used by COR and block jobs doing something with
>> a backing chain, so it shouldn’t matter in practice, but, well.
> 
> So basically just replacing the line with this?
> 
>     if ((cumulative_perms & (BLK_PERM_WRITE | BDRV_PERM_WRITE_UNCHANGED)) &&
> 
> I can do that while applying if it is what you mean.

Yes. :)

>> So, either way:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> Thanks!
> 
> Kevin
> 
>>> +        !(cumulative_perms & BLK_PERM_RESIZE))
>>> +    {
>>> +        if ((bs->total_sectors * BDRV_SECTOR_SIZE) % bs->bl.request_alignment) {
>>> +            error_setg(errp, "Cannot get 'write' permission without 'resize': "
>>> +                             "Image size is not a multiple of request "
>>> +                             "alignment");
>>> +            return -EPERM;
>>> +        }
>>> +    }
>>> +
>>>      /* Check this node */
>>>      if (!drv) {
>>>          return 0;
>>>
>>
>>
> 
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-07-17 11:37 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 14:25 [PATCH for-5.1 v2 0/2] qemu-img convert: Fix abort with unaligned image size Kevin Wolf
2020-07-16 14:26 ` [PATCH for-5.1 v2 1/2] block: Require aligned image size to avoid assertion failure Kevin Wolf
2020-07-17 11:02   ` Max Reitz
2020-07-17 11:32     ` Kevin Wolf
2020-07-17 11:36       ` Max Reitz
2020-07-16 14:26 ` [PATCH for-5.1 v2 2/2] file-posix: Allow byte-aligned O_DIRECT with NFS Kevin Wolf
2020-07-17 11:15   ` Max Reitz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.