| Message ID | 20220121170544.2049944-5-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 21.01.22 18:05, Emanuele Giuseppe Esposito wrote: > Allow writable exports to get BLK_PERM_RESIZE permission > from creation, in fuse_export_create(). > In this way, there is no need to give the permission in > fuse_do_truncate(), which might be run in an iothread. > > Permissions should be set only in the main thread, so > in any case if an iothread tries to set RESIZE, it will > be blocked. > > Also assert in fuse_do_truncate that if we give the > RESIZE permission we can then restore the original ones, > since we don't check the return value of blk_set_perm. We do, because we pass &error_abort for errp, so if an error were to occur, qemu would abort. Not that I mind adding an assertion on the return value, just noting that we omitted that kind of intentionally. Reviewed-by: Hanna Reitz <hreitz@redhat.com> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > block/export/fuse.c | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/block/export/fuse.c b/block/export/fuse.c > index 823c126d23..3c177b9e67 100644 > --- a/block/export/fuse.c > +++ b/block/export/fuse.c > @@ -86,8 +86,8 @@ static int fuse_export_create(BlockExport *blk_exp, > > assert(blk_exp_args->type == BLOCK_EXPORT_TYPE_FUSE); > > - /* For growable exports, take the RESIZE permission */ > - if (args->growable) { > + /* For growable and writable exports, take the RESIZE permission */ > + if (args->growable || blk_exp_args->writable) { > uint64_t blk_perm, blk_shared_perm; > > blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm); > @@ -392,14 +392,23 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size, > { > uint64_t blk_perm, blk_shared_perm; > BdrvRequestFlags truncate_flags = 0; > - int ret; > + bool add_resize_perm; > + int ret, ret_check; > + > + /* Growable and writable exports have a permanent RESIZE permission */ > + add_resize_perm = !exp->growable && !exp->writable; > > if (req_zero_write) { > truncate_flags |= BDRV_REQ_ZERO_WRITE; > } > > - /* Growable exports have a permanent RESIZE permission */ > - if (!exp->growable) { > + if (add_resize_perm) { > + > + if (!qemu_in_main_thread()) { > + /* Changing permissions like below only works in the main thread */ > + return -EPERM; > + } > + > blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm); > > ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE, > @@ -412,9 +421,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size, > ret = blk_truncate(exp->common.blk, size, true, prealloc, > truncate_flags, NULL); > > - if (!exp->growable) { > + if (add_resize_perm) { > /* Must succeed, because we are only giving up the RESIZE permission */ > - blk_set_perm(exp->common.blk, blk_perm, blk_shared_perm, &error_abort); > + ret_check = blk_set_perm(exp->common.blk, blk_perm, > + blk_shared_perm, &error_abort); > + assert(ret_check == 0); > } > > return ret;
On 25/01/2022 17:51, Hanna Reitz wrote: > On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote: >> Allow writable exports to get BLK_PERM_RESIZE permission >> from creation, in fuse_export_create(). >> In this way, there is no need to give the permission in >> fuse_do_truncate(), which might be run in an iothread. >> >> Permissions should be set only in the main thread, so >> in any case if an iothread tries to set RESIZE, it will >> be blocked. >> >> Also assert in fuse_do_truncate that if we give the >> RESIZE permission we can then restore the original ones, >> since we don't check the return value of blk_set_perm. > I will then just remove the last line in the commit message ("since .. blk_set_perm."). Thank you, Emanuele > We do, because we pass &error_abort for errp, so if an error were to > occur, qemu would abort. > > Not that I mind adding an assertion on the return value, just noting > that we omitted that kind of intentionally. > > Reviewed-by: Hanna Reitz <hreitz@redhat.com> > >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> block/export/fuse.c | 25 ++++++++++++++++++------- >> 1 file changed, 18 insertions(+), 7 deletions(-) >> >> diff --git a/block/export/fuse.c b/block/export/fuse.c >> index 823c126d23..3c177b9e67 100644 >> --- a/block/export/fuse.c >> +++ b/block/export/fuse.c >> @@ -86,8 +86,8 @@ static int fuse_export_create(BlockExport *blk_exp, >> assert(blk_exp_args->type == BLOCK_EXPORT_TYPE_FUSE); >> - /* For growable exports, take the RESIZE permission */ >> - if (args->growable) { >> + /* For growable and writable exports, take the RESIZE permission */ >> + if (args->growable || blk_exp_args->writable) { >> uint64_t blk_perm, blk_shared_perm; >> blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm); >> @@ -392,14 +392,23 @@ static int fuse_do_truncate(const FuseExport >> *exp, int64_t size, >> { >> uint64_t blk_perm, blk_shared_perm; >> BdrvRequestFlags truncate_flags = 0; >> - int ret; >> + bool add_resize_perm; >> + int ret, ret_check; >> + >> + /* Growable and writable exports have a permanent RESIZE >> permission */ >> + add_resize_perm = !exp->growable && !exp->writable; >> if (req_zero_write) { >> truncate_flags |= BDRV_REQ_ZERO_WRITE; >> } >> - /* Growable exports have a permanent RESIZE permission */ >> - if (!exp->growable) { >> + if (add_resize_perm) { >> + >> + if (!qemu_in_main_thread()) { >> + /* Changing permissions like below only works in the main >> thread */ >> + return -EPERM; >> + } >> + >> blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm); >> ret = blk_set_perm(exp->common.blk, blk_perm | >> BLK_PERM_RESIZE, >> @@ -412,9 +421,11 @@ static int fuse_do_truncate(const FuseExport >> *exp, int64_t size, >> ret = blk_truncate(exp->common.blk, size, true, prealloc, >> truncate_flags, NULL); >> - if (!exp->growable) { >> + if (add_resize_perm) { >> /* Must succeed, because we are only giving up the RESIZE >> permission */ >> - blk_set_perm(exp->common.blk, blk_perm, blk_shared_perm, >> &error_abort); >> + ret_check = blk_set_perm(exp->common.blk, blk_perm, >> + blk_shared_perm, &error_abort); >> + assert(ret_check == 0); >> } >> return ret; >
diff --git a/block/export/fuse.c b/block/export/fuse.c index 823c126d23..3c177b9e67 100644 --- a/block/export/fuse.c +++ b/block/export/fuse.c @@ -86,8 +86,8 @@ static int fuse_export_create(BlockExport *blk_exp, assert(blk_exp_args->type == BLOCK_EXPORT_TYPE_FUSE); - /* For growable exports, take the RESIZE permission */ - if (args->growable) { + /* For growable and writable exports, take the RESIZE permission */ + if (args->growable || blk_exp_args->writable) { uint64_t blk_perm, blk_shared_perm; blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm); @@ -392,14 +392,23 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size, { uint64_t blk_perm, blk_shared_perm; BdrvRequestFlags truncate_flags = 0; - int ret; + bool add_resize_perm; + int ret, ret_check; + + /* Growable and writable exports have a permanent RESIZE permission */ + add_resize_perm = !exp->growable && !exp->writable; if (req_zero_write) { truncate_flags |= BDRV_REQ_ZERO_WRITE; } - /* Growable exports have a permanent RESIZE permission */ - if (!exp->growable) { + if (add_resize_perm) { + + if (!qemu_in_main_thread()) { + /* Changing permissions like below only works in the main thread */ + return -EPERM; + } + blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm); ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE, @@ -412,9 +421,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size, ret = blk_truncate(exp->common.blk, size, true, prealloc, truncate_flags, NULL); - if (!exp->growable) { + if (add_resize_perm) { /* Must succeed, because we are only giving up the RESIZE permission */ - blk_set_perm(exp->common.blk, blk_perm, blk_shared_perm, &error_abort); + ret_check = blk_set_perm(exp->common.blk, blk_perm, + blk_shared_perm, &error_abort); + assert(ret_check == 0); } return ret;
Allow writable exports to get BLK_PERM_RESIZE permission from creation, in fuse_export_create(). In this way, there is no need to give the permission in fuse_do_truncate(), which might be run in an iothread. Permissions should be set only in the main thread, so in any case if an iothread tries to set RESIZE, it will be blocked. Also assert in fuse_do_truncate that if we give the RESIZE permission we can then restore the original ones, since we don't check the return value of blk_set_perm. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block/export/fuse.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)