| Message ID | 20211124064418.3120601-6-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block layer: split block APIs in global state and I/O | expand |
On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote: > Fuse logic can be classified as I/O, so there is no BQL held > during its execution. And yet, it uses blk_{get/set}_perm > functions, that are classified as BQL and clearly require > the BQL lock. Since there is no easy solution for this, > add a couple of TODOs and FIXME in the relevant sections of the > code. Well, the problem goes deeper, because we still consider them GS functions. So it’s fine for the permission function raw_handle_perm_lock() to call bdrv_get_flags(), which is a GS function, and then you still get: qemu-storage-daemon: ../block.c:6195: bdrv_get_flags: Assertion `qemu_in_main_thread()' failed. It looks like in this case making bdrv_get_flags() not a GS function would be feasible and might solve the problem, but I guess we should instead think about adding something like if (!exp->growable && !qemu_in_main_thread()) { /* Changing permissions like below only works in the main thread */ return -EPERM; } to fuse_do_truncate(). Ideally, to make up for this, we should probably take the RESIZE permission in fuse_export_create() for writable exports in iothreads. > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > block/block-backend.c | 10 ++++++++++ > block/export/fuse.c | 16 ++++++++++++++-- > 2 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 1f0bda578e..7a4b50a8f0 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -888,6 +888,11 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm, > Error **errp) > { > int ret; > + /* > + * FIXME: blk_{get/set}_perm should be always called under > + * BQL, but it is not the case right now (see block/export/fuse.c) > + */ > + /* assert(qemu_in_main_thread()); */ > > if (blk->root && !blk->disable_perm) { > ret = bdrv_child_try_set_perm(blk->root, perm, shared_perm, errp); > @@ -904,6 +909,11 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm, > > void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm) > { > + /* > + * FIXME: blk_{get/set}_perm should be always called under > + * BQL, but it is not the case right now (see block/export/fuse.c) > + */ > + /* assert(qemu_in_main_thread()); */ > *perm = blk->perm; > *shared_perm = blk->shared_perm; > } > diff --git a/block/export/fuse.c b/block/export/fuse.c > index 823c126d23..7ceb8d783b 100644 > --- a/block/export/fuse.c > +++ b/block/export/fuse.c > @@ -89,7 +89,10 @@ static int fuse_export_create(BlockExport *blk_exp, > /* For growable exports, take the RESIZE permission */ > if (args->growable) { > uint64_t blk_perm, blk_shared_perm; > - > + /* > + * FIXME: blk_{get/set}_perm should not be here, as permissions > + * should be modified only under BQL and here we are not! > + */ I believe we are, BlockExportDriver.create() is called by blk_exp_add(), which looks very much like it runs in the main thread (calling bdrv_lookup_bs(), bdrv_try_set_aio_context(), blk_new(), and whatnot). Hanna > blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm); > > ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE, > @@ -400,6 +403,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size, > > /* Growable exports have a permanent RESIZE permission */ > if (!exp->growable) { > + > + /* > + * FIXME: blk_{get/set}_perm should not be here, as permissions > + * should be modified only under BQL and here we are not! > + */ > blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm); > > ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE, > @@ -413,7 +421,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size, > truncate_flags, NULL); > > if (!exp->growable) { > - /* Must succeed, because we are only giving up the RESIZE permission */ > + /* > + * Must succeed, because we are only giving up the RESIZE permission. > + * FIXME: blk_{get/set}_perm should not be here, as permissions > + * should be modified only under BQL and here we are not! > + */ > blk_set_perm(exp->common.blk, blk_perm, blk_shared_perm, &error_abort); > } >
On 10/12/2021 15:38, Hanna Reitz wrote: > On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote: >> Fuse logic can be classified as I/O, so there is no BQL held >> during its execution. And yet, it uses blk_{get/set}_perm >> functions, that are classified as BQL and clearly require >> the BQL lock. Since there is no easy solution for this, >> add a couple of TODOs and FIXME in the relevant sections of the >> code. > > Well, the problem goes deeper, because we still consider them GS > functions. So it’s fine for the permission function > raw_handle_perm_lock() to call bdrv_get_flags(), which is a GS function, > and then you still get: > > qemu-storage-daemon: ../block.c:6195: bdrv_get_flags: Assertion > `qemu_in_main_thread()' failed. > > It looks like in this case making bdrv_get_flags() not a GS function > would be feasible and might solve the problem, but I guess we should > instead think about adding something like > > if (!exp->growable && !qemu_in_main_thread()) { > /* Changing permissions like below only works in the main thread */ > return -EPERM; > } > > to fuse_do_truncate(). > > Ideally, to make up for this, we should probably take the RESIZE > permission in fuse_export_create() for writable exports in iothreads. I think I got it. I will send the RESIZE permission fix in a separate patch. Thank you, Emanuele
On 15/12/2021 09:57, Emanuele Giuseppe Esposito wrote: > > > On 10/12/2021 15:38, Hanna Reitz wrote: >> On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote: >>> Fuse logic can be classified as I/O, so there is no BQL held >>> during its execution. And yet, it uses blk_{get/set}_perm >>> functions, that are classified as BQL and clearly require >>> the BQL lock. Since there is no easy solution for this, >>> add a couple of TODOs and FIXME in the relevant sections of the >>> code. >> >> Well, the problem goes deeper, because we still consider them GS >> functions. So it’s fine for the permission function >> raw_handle_perm_lock() to call bdrv_get_flags(), which is a GS >> function, and then you still get: >> >> qemu-storage-daemon: ../block.c:6195: bdrv_get_flags: Assertion >> `qemu_in_main_thread()' failed. Wait... Now that I read it more carefully I am confused about this. I don't think the above has to do with fuse, right? Can you share the command that makes qemu-storage-daemon fail? raw_handle_perm_lock() is currently called by these three callbacks: .bdrv_check_perm = raw_check_perm, .bdrv_set_perm = raw_set_perm, .bdrv_abort_perm_update = raw_abort_perm_update, all three function pointers are defined as GS functions. So I don't understand why is this failing. >> >> It looks like in this case making bdrv_get_flags() not a GS function >> would be feasible and might solve the problem, but I guess we should >> instead think about adding something like >> >> if (!exp->growable && !qemu_in_main_thread()) { >> /* Changing permissions like below only works in the main thread */ >> return -EPERM; >> } >> >> to fuse_do_truncate(). >> >> Ideally, to make up for this, we should probably take the RESIZE >> permission in fuse_export_create() for writable exports in iothreads. > > I think I got it. I will send the RESIZE permission fix in a separate > patch. > > Thank you, > Emanuele
On 15.12.21 11:13, Emanuele Giuseppe Esposito wrote: > > > On 15/12/2021 09:57, Emanuele Giuseppe Esposito wrote: >> >> >> On 10/12/2021 15:38, Hanna Reitz wrote: >>> On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote: >>>> Fuse logic can be classified as I/O, so there is no BQL held >>>> during its execution. And yet, it uses blk_{get/set}_perm >>>> functions, that are classified as BQL and clearly require >>>> the BQL lock. Since there is no easy solution for this, >>>> add a couple of TODOs and FIXME in the relevant sections of the >>>> code. >>> >>> Well, the problem goes deeper, because we still consider them GS >>> functions. So it’s fine for the permission function >>> raw_handle_perm_lock() to call bdrv_get_flags(), which is a GS >>> function, and then you still get: >>> >>> qemu-storage-daemon: ../block.c:6195: bdrv_get_flags: Assertion >>> `qemu_in_main_thread()' failed. > > Wait... Now that I read it more carefully I am confused about this. I > don't think the above has to do with fuse, right? It does, because the problem is that the FUSE export code (fuse_do_truncate()) calls a permission function (blk_set_perm()), and then we assume in those permission functions that they can call GS functions, like bdrv_get_flags() (called from raw_handle_perm_lock()). So in practice, the permission functions are still effectively GS functions, and just dropping the assertions from blk_set/get_perm() doesn’t really help. (This is the state on this patch; patch 7 adds an assertion in bdrv_child_try_set_perm(), and so from then on, that assertion fails before the one in bdrv_get_flags() can.) > Can you share the command that makes qemu-storage-daemon fail? Sure: $ touch /tmp/fuse-export $ storage-daemon/qemu-storage-daemon \ --object iothread,id=iothr0 \ --blockdev file,node-name=node0,filename=/tmp/fuse-export \ --export fuse,id=exp0,node-name=node0,mountpoint=/tmp/fuse-export,iothread=iothr0,writable=true \ & [1] 96997 $ truncate /tmp/fuse-export -s 1M qemu-storage-daemon: ../block.c:6197: bdrv_get_flags: Assertion `qemu_in_main_thread()' failed. truncate: failed to truncate '/tmp/fuse-export' at 1048576 bytes: Software caused connection abort truncate: failed to close '/tmp/fuse-export': Transport endpoint is not connected [1] + 96997 IOT instruction (core dumped) storage-daemon/qemu-storage-daemon --object iothread,id=iothr0 --blockdev $ fusermount -u /tmp/fuse-export Relevant part of the stacktrace: #5 0x00007f68322243a6 in __GI___assert_fail (assertion=assertion@entry=0x562380ca2f53 "qemu_in_main_thread()", file=file@entry=0x562380ca53cd "../block.c", line=line@entry=6197, function=function@entry=0x562380ca78e8 <__PRETTY_FUNCTION__.63> "bdrv_get_flags") at assert.c:101 #6 0x0000562380b64283 in bdrv_get_flags (bs=0x562382440680) at ../block.c:6197 #7 0x0000562380b68506 in bdrv_get_flags (bs=bs@entry=0x562382440680) at ../block.c:6199 #8 0x0000562380be6d18 in raw_handle_perm_lock (errp=0x7f68277103b0, new_shared=31, new_perm=11, op=RAW_PL_PREPARE, bs=0x562382440680) at ../block/file-posix.c:975 #9 raw_check_perm (bs=0x562382440680, perm=11, shared=31, errp=0x7f68277103b0) at ../block/file-posix.c:3172 #10 0x0000562380b66db5 in bdrv_drv_set_perm (errp=0x7f68277103b0, tran=0x7f68180038b0, shared_perm=31, perm=11, bs=0x562382440680) at ../block.c:2272 #11 bdrv_node_refresh_perm (errp=0x7f68277103b0, tran=0x7f68180038b0, q=0x0, bs=0x562382440680) at ../block.c:2441 #12 bdrv_list_refresh_perms (list=list@entry=0x56238243a1c0 = {...}, q=q@entry=0x0, tran=tran@entry=0x7f68180038b0, errp=errp@entry=0x7f68277103b0) at ../block.c:2479 #13 0x0000562380b67098 in bdrv_refresh_perms (bs=0x562382440680, errp=errp@entry=0x7f68277103b0) at ../block.c:2542 #14 0x0000562380b6727c in bdrv_child_try_set_perm (c=0x56238243cf70, perm=11, shared=31, errp=0x0) at ../block.c:2557 #15 0x0000562380b8632d in blk_set_perm (blk=0x56238243e7a0, perm=11, shared_perm=31, errp=errp@entry=0x0) at ../block/block-backend.c:890 #16 0x0000562380b57a7d in fuse_do_truncate (exp=exp@entry=0x56238243eb20, size=1048576, req_zero_write=req_zero_write@entry=true, prealloc=prealloc@entry=PREALLOC_MODE_OFF) at ../block/export/fuse.c:405 #17 0x0000562380b58121 in fuse_setattr (req=0x7f6818003800, inode=1, statbuf=0x7f68277104e0, to_set=8, fi=0x7f68277104b0) at ../block/export/fuse.c:476 > raw_handle_perm_lock() is currently called by these three callbacks: > .bdrv_check_perm = raw_check_perm, > .bdrv_set_perm = raw_set_perm, > .bdrv_abort_perm_update = raw_abort_perm_update, > > all three function pointers are defined as GS functions. So I don't > understand why is this failing. Because this patch explicitly allows I/O code to call blk_set/get_perm(). But as you rightly say, all permission functions are still classified as GS code, so we can’t allow it. Hanna >>> >>> It looks like in this case making bdrv_get_flags() not a GS function >>> would be feasible and might solve the problem, but I guess we should >>> instead think about adding something like >>> >>> if (!exp->growable && !qemu_in_main_thread()) { >>> /* Changing permissions like below only works in the main >>> thread */ >>> return -EPERM; >>> } >>> >>> to fuse_do_truncate(). >>> >>> Ideally, to make up for this, we should probably take the RESIZE >>> permission in fuse_export_create() for writable exports in iothreads. >> >> I think I got it. I will send the RESIZE permission fix in a separate >> patch. >> >> Thank you, >> Emanuele >
diff --git a/block/block-backend.c b/block/block-backend.c index 1f0bda578e..7a4b50a8f0 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -888,6 +888,11 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm, Error **errp) { int ret; + /* + * FIXME: blk_{get/set}_perm should be always called under + * BQL, but it is not the case right now (see block/export/fuse.c) + */ + /* assert(qemu_in_main_thread()); */ if (blk->root && !blk->disable_perm) { ret = bdrv_child_try_set_perm(blk->root, perm, shared_perm, errp); @@ -904,6 +909,11 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm, void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm) { + /* + * FIXME: blk_{get/set}_perm should be always called under + * BQL, but it is not the case right now (see block/export/fuse.c) + */ + /* assert(qemu_in_main_thread()); */ *perm = blk->perm; *shared_perm = blk->shared_perm; } diff --git a/block/export/fuse.c b/block/export/fuse.c index 823c126d23..7ceb8d783b 100644 --- a/block/export/fuse.c +++ b/block/export/fuse.c @@ -89,7 +89,10 @@ static int fuse_export_create(BlockExport *blk_exp, /* For growable exports, take the RESIZE permission */ if (args->growable) { uint64_t blk_perm, blk_shared_perm; - + /* + * FIXME: blk_{get/set}_perm should not be here, as permissions + * should be modified only under BQL and here we are not! + */ blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm); ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE, @@ -400,6 +403,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size, /* Growable exports have a permanent RESIZE permission */ if (!exp->growable) { + + /* + * FIXME: blk_{get/set}_perm should not be here, as permissions + * should be modified only under BQL and here we are not! + */ blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm); ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE, @@ -413,7 +421,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size, truncate_flags, NULL); if (!exp->growable) { - /* Must succeed, because we are only giving up the RESIZE permission */ + /* + * Must succeed, because we are only giving up the RESIZE permission. + * FIXME: blk_{get/set}_perm should not be here, as permissions + * should be modified only under BQL and here we are not! + */ blk_set_perm(exp->common.blk, blk_perm, blk_shared_perm, &error_abort); }
Fuse logic can be classified as I/O, so there is no BQL held during its execution. And yet, it uses blk_{get/set}_perm functions, that are classified as BQL and clearly require the BQL lock. Since there is no easy solution for this, add a couple of TODOs and FIXME in the relevant sections of the code. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block/block-backend.c | 10 ++++++++++ block/export/fuse.c | 16 ++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-)