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