| Message ID | 1607077703-32344-1-git-send-email-fengli@smartx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | file-posix: check the use_lock | expand |
Am 04.12.2020 um 11:28 hat Li Feng geschrieben: > When setting the file.locking = false, we shouldn't set the lock. > > Signed-off-by: Li Feng <fengli@smartx.com> This looks right to me, but can you add a test for this scenario to iotest 182? This would both demonstrate the effect of the bug (I think it would be that files are locked after reopen even with locking=off?) and make sure that we won't have a regression later. Mentioning the effect in the commit message would be good, too. Kevin > block/file-posix.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index d5fd1dbcd2..806764f7e3 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -3104,7 +3104,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared, > } > > /* Copy locks to the new fd */ > - if (s->perm_change_fd) { > + if (s->perm_change_fd && s->use_lock) { > ret = raw_apply_lock_bytes(NULL, s->perm_change_fd, perm, ~shared, > false, errp); > if (ret < 0) { > -- > 2.24.3 >
Hi Kevin, Thanks for your reply. In my scenario, my NFS server doesn't support the file lock. And when I set the file.locking = false, the Qemu still reports: qemu-system-x86_64: -drive file=/tmp/nfs/a,format=raw,cache=none,aio=native,if=none,id=drive-virtio-disk1,file.locking=on: Failed to lock byte 100 I will look at the iotest 182 and try to add a test. Thanks, Feng Li Kevin Wolf <kwolf@redhat.com> 于2020年12月4日周五 下午6:40写道： > > Am 04.12.2020 um 11:28 hat Li Feng geschrieben: > > When setting the file.locking = false, we shouldn't set the lock. > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > This looks right to me, but can you add a test for this scenario to > iotest 182? This would both demonstrate the effect of the bug (I think > it would be that files are locked after reopen even with locking=off?) > and make sure that we won't have a regression later. Mentioning the > effect in the commit message would be good, too. > > Kevin > > > block/file-posix.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > index d5fd1dbcd2..806764f7e3 100644 > > --- a/block/file-posix.c > > +++ b/block/file-posix.c > > @@ -3104,7 +3104,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared, > > } > > > > /* Copy locks to the new fd */ > > - if (s->perm_change_fd) { > > + if (s->perm_change_fd && s->use_lock) { > > ret = raw_apply_lock_bytes(NULL, s->perm_change_fd, perm, ~shared, > > false, errp); > > if (ret < 0) { > > -- > > 2.24.3 > > >
Hi Kevin, I have read the 182 test, and I'm very confused about the test. I'm not familiar with the permissions and locks in the qemu. Could you give me more tips about how to complete the test? Li Feng <fengli@smartx.com> 于2020年12月4日周五 下午6:55写道： > > Hi Kevin, > Thanks for your reply. > > In my scenario, my NFS server doesn't support the file lock. > And when I set the file.locking = false, the Qemu still reports: > qemu-system-x86_64: -drive > file=/tmp/nfs/a,format=raw,cache=none,aio=native,if=none,id=drive-virtio-disk1,file.locking=on: > Failed to lock byte 100 > > I will look at the iotest 182 and try to add a test. > > Thanks, > Feng Li > > Kevin Wolf <kwolf@redhat.com> 于2020年12月4日周五 下午6:40写道： > > > > Am 04.12.2020 um 11:28 hat Li Feng geschrieben: > > > When setting the file.locking = false, we shouldn't set the lock. > > > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > > > This looks right to me, but can you add a test for this scenario to > > iotest 182? This would both demonstrate the effect of the bug (I think > > it would be that files are locked after reopen even with locking=off?) > > and make sure that we won't have a regression later. Mentioning the > > effect in the commit message would be good, too. > > > > Kevin > > > > > block/file-posix.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > > index d5fd1dbcd2..806764f7e3 100644 > > > --- a/block/file-posix.c > > > +++ b/block/file-posix.c > > > @@ -3104,7 +3104,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared, > > > } > > > > > > /* Copy locks to the new fd */ > > > - if (s->perm_change_fd) { > > > + if (s->perm_change_fd && s->use_lock) { > > > ret = raw_apply_lock_bytes(NULL, s->perm_change_fd, perm, ~shared, > > > false, errp); > > > if (ret < 0) { > > > -- > > > 2.24.3 > > > > >
Am 07.12.2020 um 11:50 hat Feng Li geschrieben: > Hi Kevin, > > I have read the 182 test, and I'm very confused about the test. > I'm not familiar with the permissions and locks in the qemu. > Could you give me more tips about how to complete the test? Hm, actually, to produce a failure, we would have to have a filesystem in the host that doesn't support locks. I don't even know how to get such a filesystem manually, and it's probably completely impossible in a test case without root permissions. So maybe just add a more detailed description of the bug to the commit message, and we'll have to apply it without a test. Kevin > Li Feng <fengli@smartx.com> 于2020年12月4日周五 下午6:55写道： > > > > Hi Kevin, > > Thanks for your reply. > > > > In my scenario, my NFS server doesn't support the file lock. > > And when I set the file.locking = false, the Qemu still reports: > > qemu-system-x86_64: -drive > > file=/tmp/nfs/a,format=raw,cache=none,aio=native,if=none,id=drive-virtio-disk1,file.locking=on: > > Failed to lock byte 100 > > > > I will look at the iotest 182 and try to add a test. > > > > Thanks, > > Feng Li > > > > Kevin Wolf <kwolf@redhat.com> 于2020年12月4日周五 下午6:40写道： > > > > > > Am 04.12.2020 um 11:28 hat Li Feng geschrieben: > > > > When setting the file.locking = false, we shouldn't set the lock. > > > > > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > > > > > This looks right to me, but can you add a test for this scenario to > > > iotest 182? This would both demonstrate the effect of the bug (I think > > > it would be that files are locked after reopen even with locking=off?) > > > and make sure that we won't have a regression later. Mentioning the > > > effect in the commit message would be good, too. > > > > > > Kevin > > > > > > > block/file-posix.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > > > index d5fd1dbcd2..806764f7e3 100644 > > > > --- a/block/file-posix.c > > > > +++ b/block/file-posix.c > > > > @@ -3104,7 +3104,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared, > > > > } > > > > > > > > /* Copy locks to the new fd */ > > > > - if (s->perm_change_fd) { > > > > + if (s->perm_change_fd && s->use_lock) { > > > > ret = raw_apply_lock_bytes(NULL, s->perm_change_fd, perm, ~shared, > > > > false, errp); > > > > if (ret < 0) { > > > > -- > > > > 2.24.3 > > > > > > > >
Hi Kevin, OK, thanks for your reply, I will send out the v2 patch. Thanks, Feng Li Kevin Wolf <kwolf@redhat.com> 于2020年12月7日周一 下午7:17写道： > > Am 07.12.2020 um 11:50 hat Feng Li geschrieben: > > Hi Kevin, > > > > I have read the 182 test, and I'm very confused about the test. > > I'm not familiar with the permissions and locks in the qemu. > > Could you give me more tips about how to complete the test? > > Hm, actually, to produce a failure, we would have to have a filesystem > in the host that doesn't support locks. I don't even know how to get > such a filesystem manually, and it's probably completely impossible in a > test case without root permissions. > > So maybe just add a more detailed description of the bug to the commit > message, and we'll have to apply it without a test. > > Kevin > > > Li Feng <fengli@smartx.com> 于2020年12月4日周五 下午6:55写道： > > > > > > Hi Kevin, > > > Thanks for your reply. > > > > > > In my scenario, my NFS server doesn't support the file lock. > > > And when I set the file.locking = false, the Qemu still reports: > > > qemu-system-x86_64: -drive > > > file=/tmp/nfs/a,format=raw,cache=none,aio=native,if=none,id=drive-virtio-disk1,file.locking=on: > > > Failed to lock byte 100 > > > > > > I will look at the iotest 182 and try to add a test. > > > > > > Thanks, > > > Feng Li > > > > > > Kevin Wolf <kwolf@redhat.com> 于2020年12月4日周五 下午6:40写道： > > > > > > > > Am 04.12.2020 um 11:28 hat Li Feng geschrieben: > > > > > When setting the file.locking = false, we shouldn't set the lock. > > > > > > > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > > > > > > > This looks right to me, but can you add a test for this scenario to > > > > iotest 182? This would both demonstrate the effect of the bug (I think > > > > it would be that files are locked after reopen even with locking=off?) > > > > and make sure that we won't have a regression later. Mentioning the > > > > effect in the commit message would be good, too. > > > > > > > > Kevin > > > > > > > > > block/file-posix.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > > > > index d5fd1dbcd2..806764f7e3 100644 > > > > > --- a/block/file-posix.c > > > > > +++ b/block/file-posix.c > > > > > @@ -3104,7 +3104,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared, > > > > > } > > > > > > > > > > /* Copy locks to the new fd */ > > > > > - if (s->perm_change_fd) { > > > > > + if (s->perm_change_fd && s->use_lock) { > > > > > ret = raw_apply_lock_bytes(NULL, s->perm_change_fd, perm, ~shared, > > > > > false, errp); > > > > > if (ret < 0) { > > > > > -- > > > > > 2.24.3 > > > > > > > > > > > >
diff --git a/block/file-posix.c b/block/file-posix.c index d5fd1dbcd2..806764f7e3 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -3104,7 +3104,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared, } /* Copy locks to the new fd */ - if (s->perm_change_fd) { + if (s->perm_change_fd && s->use_lock) { ret = raw_apply_lock_bytes(NULL, s->perm_change_fd, perm, ~shared, false, errp); if (ret < 0) {
When setting the file.locking = false, we shouldn't set the lock. Signed-off-by: Li Feng <fengli@smartx.com> --- block/file-posix.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)