Am 06.04.2017 um 23:29 hat Richard W.M. Jones geschrieben: > On Thu, Apr 06, 2017 at 04:23:19PM -0500, Eric Blake wrote: > > On 04/06/2017 04:15 PM, Richard W.M. Jones wrote: > > > > >>>> +++ b/block/io.c > > >>>> @@ -945,6 +945,8 @@ static int coroutine_fn > > >>>> bdrv_co_do_copy_on_readv(BdrvChild *child, > > >>>> size_t skip_bytes; > > >>>> int ret; > > >>>> > > >>>> + assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE)); > > >>> > > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1439922 > > > > > > There is also a minimal reproducer in comment 2. > > > > If it helps, backtrace shows: > > > > #4 0x0000555555c09f10 in bdrv_co_do_copy_on_readv > > (child=0x555556990180, offset=0, bytes=512, qiov=0x7fffffffd0c0) at > > block/io.c:948 > > #5 0x0000555555c0a33d in bdrv_aligned_preadv (child=0x555556990180, > > req=0x7fffb6dffec0, offset=0, bytes=512, align=1, qiov=0x7fffffffd0c0, > > flags=1) > > at block/io.c:1058 > > #6 0x0000555555c0a8c3 in bdrv_co_preadv (child=0x555556990180, > > offset=0, bytes=512, qiov=0x7fffffffd0c0, flags=BDRV_REQ_COPY_ON_READ) > > at block/io.c:1166 > > #7 0x0000555555bf7ccf in blk_co_preadv (blk=0x555556a2bc20, offset=0, > > bytes=512, qiov=0x7fffffffd0c0, flags=0) at block/block-backend.c:927 > > #8 0x0000555555bf7e19 in blk_read_entry (opaque=0x7fffffffd0e0) > > at block/block-backend.c:974 > > #9 0x0000555555cc3015 in coroutine_trampoline (i0=1485566720, i1=21845) > > at util/coroutine-ucontext.c:79 > > The patch that fixes this is simply: > > diff --git a/block/io.c b/block/io.c > index 2709a7007f..4f1a730e45 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -945,8 +945,6 @@ static int coroutine_fn > bdrv_co_do_copy_on_readv(BdrvChild *child, > size_t skip_bytes; > int ret; > > - assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE)); > - > /* Cover entire cluster so no additional backing file I/O is required > when > * allocating cluster in the image file. > */ > > It has no ill effects that I can see, and fixes the libguestfs test > suite, but I'm assuming the assert was added for a reason so I'm not > proposing this as a patch.
I think for the moment this is actually the best solution. Copy on read is currently implemented in a way that we can't do things properly with respect to permissions, so we need to bypass the permission system. We can't require the callers to have write permissions for a read request, this would have to be handled internally by the COR code. As long as it is still implemented directly in block/io.c instead of a separate filter driver, the COR code doesn't have a BdrvChild and therefore can't have its own set of permissions on the node. Kevin