| Message ID | 1607432377-87084-1-git-send-email-fengli@smartx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | file-posix: detect the lock using the real file | expand |
On Tue, Dec 08, 2020 at 08:59:37PM +0800, Li Feng wrote: > This patch addresses this issue: > When accessing a volume on an NFS filesystem without supporting the file lock, > tools, like qemu-img, will complain "Failed to lock byte 100". > > In the original code, the qemu_has_ofd_lock will test the lock on the > "/dev/null" pseudo-file. Actually, the file.locking is per-drive property, > which depends on the underlay filesystem. IIUC, the problem you're describing is one of whether the filesystem supports fcntl locking at all, which is indeed a per-FS check. The QEMU code being changed though is just about detecting whether the host OS supports OFD to not, which is supposed to be a kernel level feature applied universally to all FS types. > > In this patch, make the 'qemu_has_ofd_lock' with a filename be more generic > and reasonable. > > Signed-off-by: Li Feng <fengli@smartx.com> > --- > block/file-posix.c | 32 +++++++++++++++------------- > include/qemu/osdep.h | 2 +- > tests/test-image-locking.c | 2 +- > util/osdep.c | 43 ++++++++++++++++++++++++-------------- > 4 files changed, 47 insertions(+), 32 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 806764f7e3..03be1b188c 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -595,7 +595,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > switch (locking) { > case ON_OFF_AUTO_ON: > s->use_lock = true; > - if (!qemu_has_ofd_lock()) { > + if (!qemu_has_ofd_lock(filename)) { > warn_report("File lock requested but OFD locking syscall is " > "unavailable, falling back to POSIX file locks"); > error_printf("Due to the implementation, locks can be lost " > @@ -606,7 +606,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > s->use_lock = false; > break; > case ON_OFF_AUTO_AUTO: > - s->use_lock = qemu_has_ofd_lock(); > + s->use_lock = qemu_has_ofd_lock(filename); > break; > default: > abort(); > @@ -2388,6 +2388,7 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp) > int fd; > uint64_t perm, shared; > int result = 0; > + bool use_lock; > > /* Validate options and set default values */ > assert(options->driver == BLOCKDEV_DRIVER_FILE); > @@ -2428,19 +2429,22 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp) > perm = BLK_PERM_WRITE | BLK_PERM_RESIZE; > shared = BLK_PERM_ALL & ~BLK_PERM_RESIZE; > > - /* Step one: Take locks */ > - result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp); > - if (result < 0) { > - goto out_close; > - } > + use_lock = qemu_has_ofd_lock(file_opts->filename); > + if (use_lock) { > + /* Step one: Take locks */ > + result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp); > + if (result < 0) { > + goto out_close; > + } > > - /* Step two: Check that nobody else has taken conflicting locks */ > - result = raw_check_lock_bytes(fd, perm, shared, errp); > - if (result < 0) { > - error_append_hint(errp, > - "Is another process using the image [%s]?\n", > - file_opts->filename); > - goto out_unlock; > + /* Step two: Check that nobody else has taken conflicting locks */ > + result = raw_check_lock_bytes(fd, perm, shared, errp); > + if (result < 0) { > + error_append_hint(errp, > + "Is another process using the image [%s]?\n", > + file_opts->filename); > + goto out_unlock; > + } > } > > /* Clear the file by truncating it to 0 */ > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index f9ec8c84e9..349adad465 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -512,7 +512,7 @@ int qemu_dup(int fd); > int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive); > int qemu_unlock_fd(int fd, int64_t start, int64_t len); > int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive); > -bool qemu_has_ofd_lock(void); > +bool qemu_has_ofd_lock(const char *filename); > #endif > > #if defined(__HAIKU__) && defined(__i386__) > diff --git a/tests/test-image-locking.c b/tests/test-image-locking.c > index ba057bd66c..3e80246081 100644 > --- a/tests/test-image-locking.c > +++ b/tests/test-image-locking.c > @@ -149,7 +149,7 @@ int main(int argc, char **argv) > > g_test_init(&argc, &argv, NULL); > > - if (qemu_has_ofd_lock()) { > + if (qemu_has_ofd_lock(NULL)) { > g_test_add_func("/image-locking/basic", test_image_locking_basic); > g_test_add_func("/image-locking/set-perm-abort", test_set_perm_abort); > } > diff --git a/util/osdep.c b/util/osdep.c > index 66d01b9160..e7e502edd1 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -42,6 +42,7 @@ extern int madvise(char *, size_t, int); > static bool fips_enabled = false; > > static const char *hw_version = QEMU_HW_VERSION; > +static const char *null_device = "/dev/null"; > > int socket_set_cork(int fd, int v) > { > @@ -187,11 +188,10 @@ static int qemu_parse_fdset(const char *param) > return qemu_parse_fd(param); > } > > -static void qemu_probe_lock_ops(void) > +static void qemu_probe_lock_ops_fd(int fd) > { > if (fcntl_op_setlk == -1) { > #ifdef F_OFD_SETLK > - int fd; > int ret; > struct flock fl = { > .l_whence = SEEK_SET, > @@ -200,17 +200,7 @@ static void qemu_probe_lock_ops(void) > .l_type = F_WRLCK, > }; > > - fd = open("/dev/null", O_RDWR); > - if (fd < 0) { > - fprintf(stderr, > - "Failed to open /dev/null for OFD lock probing: %s\n", > - strerror(errno)); > - fcntl_op_setlk = F_SETLK; > - fcntl_op_getlk = F_GETLK; > - return; > - } > ret = fcntl(fd, F_OFD_GETLK, &fl); > - close(fd); > if (!ret) { > fcntl_op_setlk = F_OFD_SETLK; > fcntl_op_getlk = F_OFD_GETLK; > @@ -225,9 +215,30 @@ static void qemu_probe_lock_ops(void) > } > } > > -bool qemu_has_ofd_lock(void) > +static void qemu_probe_lock_ops(const char *filename) > +{ > + int fd; > + if (filename) { > + fd = open(filename, O_RDWR); > + } else { > + fd = open(null_device, O_RDONLY); > + } > + if (fd < 0) { > + fprintf(stderr, > + "Failed to open %s for OFD lock probing: %s\n", > + filename ? filename : null_device, > + strerror(errno)); > + fcntl_op_setlk = F_SETLK; > + fcntl_op_getlk = F_GETLK; > + return; > + } > + qemu_probe_lock_ops_fd(fd); > + close(fd); > +} > + This method now does a test whose results will vary based on the filename passed in, but it is updating a global variable to say whether to use OFD locks. This is looks badly broken when using files across different filesystems. IMHO the raw_co_create method just needs to use a dedicated method to check whether fcntl locks are supposed, and all this broken refactoring of the OFD check should be removed. > +bool qemu_has_ofd_lock(const char *filename) > { > - qemu_probe_lock_ops(); > + qemu_probe_lock_ops(filename); > #ifdef F_OFD_SETLK > return fcntl_op_setlk == F_OFD_SETLK; > #else > @@ -244,7 +255,7 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type) > .l_len = len, > .l_type = fl_type, > }; > - qemu_probe_lock_ops(); > + qemu_probe_lock_ops_fd(fd); > do { > ret = fcntl(fd, fcntl_op_setlk, &fl); > } while (ret == -1 && errno == EINTR); > @@ -270,7 +281,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive) > .l_len = len, > .l_type = exclusive ? F_WRLCK : F_RDLCK, > }; > - qemu_probe_lock_ops(); > + qemu_probe_lock_ops_fd(fd); > ret = fcntl(fd, fcntl_op_getlk, &fl); > if (ret == -1) { > return -errno; > -- > 2.24.3 > > Regards, Daniel
Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > This patch addresses this issue: > When accessing a volume on an NFS filesystem without supporting the file lock, > tools, like qemu-img, will complain "Failed to lock byte 100". > > In the original code, the qemu_has_ofd_lock will test the lock on the > "/dev/null" pseudo-file. Actually, the file.locking is per-drive property, > which depends on the underlay filesystem. > > In this patch, make the 'qemu_has_ofd_lock' with a filename be more generic > and reasonable. > > Signed-off-by: Li Feng <fengli@smartx.com> Do you know any way how I could configure either the NFS server or the NFS client such that locking would fail? For any patch related to this, it would be good if I could even test the scenario. For this specific patch, I think Daniel has already provided a good explanation of the fundamental problems it has. Kevin
Daniel P. Berrangé <berrange@redhat.com> 于2020年12月8日周二 下午9:45写道： > > On Tue, Dec 08, 2020 at 08:59:37PM +0800, Li Feng wrote: > > This patch addresses this issue: > > When accessing a volume on an NFS filesystem without supporting the file lock, > > tools, like qemu-img, will complain "Failed to lock byte 100". > > > > In the original code, the qemu_has_ofd_lock will test the lock on the > > "/dev/null" pseudo-file. Actually, the file.locking is per-drive property, > > which depends on the underlay filesystem. > > IIUC, the problem you're describing is one of whether the filesystem > supports fcntl locking at all, which is indeed a per-FS check. > > The QEMU code being changed though is just about detecting whether > the host OS supports OFD to not, which is supposed to be a kernel > level feature applied universally to all FS types. > > > > > In this patch, make the 'qemu_has_ofd_lock' with a filename be more generic > > and reasonable. > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > --- > > block/file-posix.c | 32 +++++++++++++++------------- > > include/qemu/osdep.h | 2 +- > > tests/test-image-locking.c | 2 +- > > util/osdep.c | 43 ++++++++++++++++++++++++-------------- > > 4 files changed, 47 insertions(+), 32 deletions(-) > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > index 806764f7e3..03be1b188c 100644 > > --- a/block/file-posix.c > > +++ b/block/file-posix.c > > @@ -595,7 +595,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > > switch (locking) { > > case ON_OFF_AUTO_ON: > > s->use_lock = true; > > - if (!qemu_has_ofd_lock()) { > > + if (!qemu_has_ofd_lock(filename)) { > > warn_report("File lock requested but OFD locking syscall is " > > "unavailable, falling back to POSIX file locks"); > > error_printf("Due to the implementation, locks can be lost " > > @@ -606,7 +606,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > > s->use_lock = false; > > break; > > case ON_OFF_AUTO_AUTO: > > - s->use_lock = qemu_has_ofd_lock(); > > + s->use_lock = qemu_has_ofd_lock(filename); > > break; > > default: > > abort(); > > @@ -2388,6 +2388,7 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp) > > int fd; > > uint64_t perm, shared; > > int result = 0; > > + bool use_lock; > > > > /* Validate options and set default values */ > > assert(options->driver == BLOCKDEV_DRIVER_FILE); > > @@ -2428,19 +2429,22 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp) > > perm = BLK_PERM_WRITE | BLK_PERM_RESIZE; > > shared = BLK_PERM_ALL & ~BLK_PERM_RESIZE; > > > > - /* Step one: Take locks */ > > - result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp); > > - if (result < 0) { > > - goto out_close; > > - } > > + use_lock = qemu_has_ofd_lock(file_opts->filename); > > + if (use_lock) { > > + /* Step one: Take locks */ > > + result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp); > > + if (result < 0) { > > + goto out_close; > > + } > > > > - /* Step two: Check that nobody else has taken conflicting locks */ > > - result = raw_check_lock_bytes(fd, perm, shared, errp); > > - if (result < 0) { > > - error_append_hint(errp, > > - "Is another process using the image [%s]?\n", > > - file_opts->filename); > > - goto out_unlock; > > + /* Step two: Check that nobody else has taken conflicting locks */ > > + result = raw_check_lock_bytes(fd, perm, shared, errp); > > + if (result < 0) { > > + error_append_hint(errp, > > + "Is another process using the image [%s]?\n", > > + file_opts->filename); > > + goto out_unlock; > > + } > > } > > > > /* Clear the file by truncating it to 0 */ > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > > index f9ec8c84e9..349adad465 100644 > > --- a/include/qemu/osdep.h > > +++ b/include/qemu/osdep.h > > @@ -512,7 +512,7 @@ int qemu_dup(int fd); > > int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive); > > int qemu_unlock_fd(int fd, int64_t start, int64_t len); > > int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive); > > -bool qemu_has_ofd_lock(void); > > +bool qemu_has_ofd_lock(const char *filename); > > #endif > > > > #if defined(__HAIKU__) && defined(__i386__) > > diff --git a/tests/test-image-locking.c b/tests/test-image-locking.c > > index ba057bd66c..3e80246081 100644 > > --- a/tests/test-image-locking.c > > +++ b/tests/test-image-locking.c > > @@ -149,7 +149,7 @@ int main(int argc, char **argv) > > > > g_test_init(&argc, &argv, NULL); > > > > - if (qemu_has_ofd_lock()) { > > + if (qemu_has_ofd_lock(NULL)) { > > g_test_add_func("/image-locking/basic", test_image_locking_basic); > > g_test_add_func("/image-locking/set-perm-abort", test_set_perm_abort); > > } > > diff --git a/util/osdep.c b/util/osdep.c > > index 66d01b9160..e7e502edd1 100644 > > --- a/util/osdep.c > > +++ b/util/osdep.c > > @@ -42,6 +42,7 @@ extern int madvise(char *, size_t, int); > > static bool fips_enabled = false; > > > > static const char *hw_version = QEMU_HW_VERSION; > > +static const char *null_device = "/dev/null"; > > > > int socket_set_cork(int fd, int v) > > { > > @@ -187,11 +188,10 @@ static int qemu_parse_fdset(const char *param) > > return qemu_parse_fd(param); > > } > > > > -static void qemu_probe_lock_ops(void) > > +static void qemu_probe_lock_ops_fd(int fd) > > { > > if (fcntl_op_setlk == -1) { > > #ifdef F_OFD_SETLK > > - int fd; > > int ret; > > struct flock fl = { > > .l_whence = SEEK_SET, > > @@ -200,17 +200,7 @@ static void qemu_probe_lock_ops(void) > > .l_type = F_WRLCK, > > }; > > > > - fd = open("/dev/null", O_RDWR); > > - if (fd < 0) { > > - fprintf(stderr, > > - "Failed to open /dev/null for OFD lock probing: %s\n", > > - strerror(errno)); > > - fcntl_op_setlk = F_SETLK; > > - fcntl_op_getlk = F_GETLK; > > - return; > > - } > > ret = fcntl(fd, F_OFD_GETLK, &fl); > > - close(fd); > > if (!ret) { > > fcntl_op_setlk = F_OFD_SETLK; > > fcntl_op_getlk = F_OFD_GETLK; > > @@ -225,9 +215,30 @@ static void qemu_probe_lock_ops(void) > > } > > } > > > > -bool qemu_has_ofd_lock(void) > > +static void qemu_probe_lock_ops(const char *filename) > > +{ > > + int fd; > > + if (filename) { > > + fd = open(filename, O_RDWR); > > + } else { > > + fd = open(null_device, O_RDONLY); > > + } > > + if (fd < 0) { > > + fprintf(stderr, > > + "Failed to open %s for OFD lock probing: %s\n", > > + filename ? filename : null_device, > > + strerror(errno)); > > + fcntl_op_setlk = F_SETLK; > > + fcntl_op_getlk = F_GETLK; > > + return; > > + } > > + qemu_probe_lock_ops_fd(fd); > > + close(fd); > > +} > > + > > This method now does a test whose results will vary based on the > filename passed in, but it is updating a global variable to say > whether to use OFD locks. This is looks badly broken when using > files across different filesystems. Daniel, thanks for your reply. I understand your concern, however, this global variable actually maybe not suitable if files are across different filesystems. > > IMHO the raw_co_create method just needs to use a dedicated method > to check whether fcntl locks are supposed, and all this broken > refactoring of the OFD check should be removed. > OK, I will just keep the fixed part in v2. > > +bool qemu_has_ofd_lock(const char *filename) > > { > > - qemu_probe_lock_ops(); > > + qemu_probe_lock_ops(filename); > > #ifdef F_OFD_SETLK > > return fcntl_op_setlk == F_OFD_SETLK; > > #else > > @@ -244,7 +255,7 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type) > > .l_len = len, > > .l_type = fl_type, > > }; > > - qemu_probe_lock_ops(); > > + qemu_probe_lock_ops_fd(fd); > > do { > > ret = fcntl(fd, fcntl_op_setlk, &fl); > > } while (ret == -1 && errno == EINTR); > > @@ -270,7 +281,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive) > > .l_len = len, > > .l_type = exclusive ? F_WRLCK : F_RDLCK, > > }; > > - qemu_probe_lock_ops(); > > + qemu_probe_lock_ops_fd(fd); > > ret = fcntl(fd, fcntl_op_getlk, &fl); > > if (ret == -1) { > > return -errno; > > -- > > 2.24.3 > > > > > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
Kevin Wolf <kwolf@redhat.com> 于2020年12月8日周二 下午10:38写道： > > Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > > This patch addresses this issue: > > When accessing a volume on an NFS filesystem without supporting the file lock, > > tools, like qemu-img, will complain "Failed to lock byte 100". > > > > In the original code, the qemu_has_ofd_lock will test the lock on the > > "/dev/null" pseudo-file. Actually, the file.locking is per-drive property, > > which depends on the underlay filesystem. > > > > In this patch, make the 'qemu_has_ofd_lock' with a filename be more generic > > and reasonable. > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > Do you know any way how I could configure either the NFS server or the > NFS client such that locking would fail? For any patch related to this, > it would be good if I could even test the scenario. > Hi Kevin, currently our SmartX ZBS storage NFS server doesn't support the file lock and the lock operation will return failure. I have tried the kernel NFS server, and it works well. I don't have more kinds of NFS servers. > For this specific patch, I think Daniel has already provided a good > explanation of the fundamental problems it has. > > Kevin >
On Tue, Dec 08, 2020 at 03:38:22PM +0100, Kevin Wolf wrote: > Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > > This patch addresses this issue: > > When accessing a volume on an NFS filesystem without supporting the file lock, > > tools, like qemu-img, will complain "Failed to lock byte 100". > > > > In the original code, the qemu_has_ofd_lock will test the lock on the > > "/dev/null" pseudo-file. Actually, the file.locking is per-drive property, > > which depends on the underlay filesystem. > > > > In this patch, make the 'qemu_has_ofd_lock' with a filename be more generic > > and reasonable. > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > Do you know any way how I could configure either the NFS server or the > NFS client such that locking would fail? For any patch related to this, > it would be good if I could even test the scenario. One could write a qtest that uses an LD_PRELOAD to replace the standard glibc fcntl() function with one that returns an error for locking commands. Regards, Daniel
Am 09.12.2020 um 10:33 hat Daniel P. Berrangé geschrieben: > On Tue, Dec 08, 2020 at 03:38:22PM +0100, Kevin Wolf wrote: > > Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > > > This patch addresses this issue: > > > When accessing a volume on an NFS filesystem without supporting the file lock, > > > tools, like qemu-img, will complain "Failed to lock byte 100". > > > > > > In the original code, the qemu_has_ofd_lock will test the lock on the > > > "/dev/null" pseudo-file. Actually, the file.locking is per-drive property, > > > which depends on the underlay filesystem. > > > > > > In this patch, make the 'qemu_has_ofd_lock' with a filename be more generic > > > and reasonable. > > > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > > > Do you know any way how I could configure either the NFS server or the > > NFS client such that locking would fail? For any patch related to this, > > it would be good if I could even test the scenario. > > One could write a qtest that uses an LD_PRELOAD to replace the standard > glibc fcntl() function with one that returns an error for locking commands. Sounds a bit ugly, but for regression testing it could make sense. However, part of the testing would be to verify that we our checks actually match the kernel code, which this approach couldn't solve. Kevin
Kevin Wolf <kwolf@redhat.com> 于2020年12月10日周四 上午1:43写道： > > Am 09.12.2020 um 10:33 hat Daniel P. Berrangé geschrieben: > > On Tue, Dec 08, 2020 at 03:38:22PM +0100, Kevin Wolf wrote: > > > Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > > > > This patch addresses this issue: > > > > When accessing a volume on an NFS filesystem without supporting the file lock, > > > > tools, like qemu-img, will complain "Failed to lock byte 100". > > > > > > > > In the original code, the qemu_has_ofd_lock will test the lock on the > > > > "/dev/null" pseudo-file. Actually, the file.locking is per-drive property, > > > > which depends on the underlay filesystem. > > > > > > > > In this patch, make the 'qemu_has_ofd_lock' with a filename be more generic > > > > and reasonable. > > > > > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > > > > > Do you know any way how I could configure either the NFS server or the > > > NFS client such that locking would fail? For any patch related to this, > > > it would be good if I could even test the scenario. > > > > One could write a qtest that uses an LD_PRELOAD to replace the standard > > glibc fcntl() function with one that returns an error for locking commands. > > Sounds a bit ugly, but for regression testing it could make sense. > > However, part of the testing would be to verify that we our checks > actually match the kernel code, which this approach couldn't solve. > Hi, Kevin and Daniel. How about this patch? I think it's very straightforward. Except we need a mock syscall test case. > Kevin >
On Thu, Dec 10, 2020 at 10:56:59PM +0800, Li Feng wrote: > Kevin Wolf <kwolf@redhat.com> 于2020年12月10日周四 上午1:43写道： > > > > Am 09.12.2020 um 10:33 hat Daniel P. Berrangé geschrieben: > > > On Tue, Dec 08, 2020 at 03:38:22PM +0100, Kevin Wolf wrote: > > > > Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > > > > > This patch addresses this issue: > > > > > When accessing a volume on an NFS filesystem without supporting the file lock, > > > > > tools, like qemu-img, will complain "Failed to lock byte 100". > > > > > > > > > > In the original code, the qemu_has_ofd_lock will test the lock on the > > > > > "/dev/null" pseudo-file. Actually, the file.locking is per-drive property, > > > > > which depends on the underlay filesystem. > > > > > > > > > > In this patch, make the 'qemu_has_ofd_lock' with a filename be more generic > > > > > and reasonable. > > > > > > > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > > > > > > > Do you know any way how I could configure either the NFS server or the > > > > NFS client such that locking would fail? For any patch related to this, > > > > it would be good if I could even test the scenario. > > > > > > One could write a qtest that uses an LD_PRELOAD to replace the standard > > > glibc fcntl() function with one that returns an error for locking commands. > > > > Sounds a bit ugly, but for regression testing it could make sense. > > > > However, part of the testing would be to verify that we our checks > > actually match the kernel code, which this approach couldn't solve. > > > Hi, Kevin and Daniel. > > How about this patch? I think it's very straightforward. > Except we need a mock syscall test case. You don't seem to have attached any patch here. Regards, Daniel
My mistake, you are not on the receiver list of my v2. I use the get_maintainer.sh to generate the cc list. I will resend it to you. Daniel P. Berrangé <berrange@redhat.com> 于2020年12月10日周四 下午11:29写道： > > On Thu, Dec 10, 2020 at 10:56:59PM +0800, Li Feng wrote: > > Kevin Wolf <kwolf@redhat.com> 于2020年12月10日周四 上午1:43写道： > > > > > > Am 09.12.2020 um 10:33 hat Daniel P. Berrangé geschrieben: > > > > On Tue, Dec 08, 2020 at 03:38:22PM +0100, Kevin Wolf wrote: > > > > > Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > > > > > > This patch addresses this issue: > > > > > > When accessing a volume on an NFS filesystem without supporting the file lock, > > > > > > tools, like qemu-img, will complain "Failed to lock byte 100". > > > > > > > > > > > > In the original code, the qemu_has_ofd_lock will test the lock on the > > > > > > "/dev/null" pseudo-file. Actually, the file.locking is per-drive property, > > > > > > which depends on the underlay filesystem. > > > > > > > > > > > > In this patch, make the 'qemu_has_ofd_lock' with a filename be more generic > > > > > > and reasonable. > > > > > > > > > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > > > > > > > > > Do you know any way how I could configure either the NFS server or the > > > > > NFS client such that locking would fail? For any patch related to this, > > > > > it would be good if I could even test the scenario. > > > > > > > > One could write a qtest that uses an LD_PRELOAD to replace the standard > > > > glibc fcntl() function with one that returns an error for locking commands. > > > > > > Sounds a bit ugly, but for regression testing it could make sense. > > > > > > However, part of the testing would be to verify that we our checks > > > actually match the kernel code, which this approach couldn't solve. > > > > > Hi, Kevin and Daniel. > > > > How about this patch? I think it's very straightforward. > > Except we need a mock syscall test case. > > You don't seem to have attached any patch here. > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On Thu, Dec 10, 2020 at 11:53:09PM +0800, Feng Li wrote: > My mistake, you are not on the receiver list of my v2. > I use the get_maintainer.sh to generate the cc list. > I will resend it to you. No, need. I've seen 2, but didn't think you were referring to that as it still has most of the flaws I pointed out in v1 Regards, Daniel
diff --git a/block/file-posix.c b/block/file-posix.c index 806764f7e3..03be1b188c 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -595,7 +595,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, switch (locking) { case ON_OFF_AUTO_ON: s->use_lock = true; - if (!qemu_has_ofd_lock()) { + if (!qemu_has_ofd_lock(filename)) { warn_report("File lock requested but OFD locking syscall is " "unavailable, falling back to POSIX file locks"); error_printf("Due to the implementation, locks can be lost " @@ -606,7 +606,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, s->use_lock = false; break; case ON_OFF_AUTO_AUTO: - s->use_lock = qemu_has_ofd_lock(); + s->use_lock = qemu_has_ofd_lock(filename); break; default: abort(); @@ -2388,6 +2388,7 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp) int fd; uint64_t perm, shared; int result = 0; + bool use_lock; /* Validate options and set default values */ assert(options->driver == BLOCKDEV_DRIVER_FILE); @@ -2428,19 +2429,22 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp) perm = BLK_PERM_WRITE | BLK_PERM_RESIZE; shared = BLK_PERM_ALL & ~BLK_PERM_RESIZE; - /* Step one: Take locks */ - result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp); - if (result < 0) { - goto out_close; - } + use_lock = qemu_has_ofd_lock(file_opts->filename); + if (use_lock) { + /* Step one: Take locks */ + result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp); + if (result < 0) { + goto out_close; + } - /* Step two: Check that nobody else has taken conflicting locks */ - result = raw_check_lock_bytes(fd, perm, shared, errp); - if (result < 0) { - error_append_hint(errp, - "Is another process using the image [%s]?\n", - file_opts->filename); - goto out_unlock; + /* Step two: Check that nobody else has taken conflicting locks */ + result = raw_check_lock_bytes(fd, perm, shared, errp); + if (result < 0) { + error_append_hint(errp, + "Is another process using the image [%s]?\n", + file_opts->filename); + goto out_unlock; + } } /* Clear the file by truncating it to 0 */ diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index f9ec8c84e9..349adad465 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -512,7 +512,7 @@ int qemu_dup(int fd); int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive); int qemu_unlock_fd(int fd, int64_t start, int64_t len); int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive); -bool qemu_has_ofd_lock(void); +bool qemu_has_ofd_lock(const char *filename); #endif #if defined(__HAIKU__) && defined(__i386__) diff --git a/tests/test-image-locking.c b/tests/test-image-locking.c index ba057bd66c..3e80246081 100644 --- a/tests/test-image-locking.c +++ b/tests/test-image-locking.c @@ -149,7 +149,7 @@ int main(int argc, char **argv) g_test_init(&argc, &argv, NULL); - if (qemu_has_ofd_lock()) { + if (qemu_has_ofd_lock(NULL)) { g_test_add_func("/image-locking/basic", test_image_locking_basic); g_test_add_func("/image-locking/set-perm-abort", test_set_perm_abort); } diff --git a/util/osdep.c b/util/osdep.c index 66d01b9160..e7e502edd1 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -42,6 +42,7 @@ extern int madvise(char *, size_t, int); static bool fips_enabled = false; static const char *hw_version = QEMU_HW_VERSION; +static const char *null_device = "/dev/null"; int socket_set_cork(int fd, int v) { @@ -187,11 +188,10 @@ static int qemu_parse_fdset(const char *param) return qemu_parse_fd(param); } -static void qemu_probe_lock_ops(void) +static void qemu_probe_lock_ops_fd(int fd) { if (fcntl_op_setlk == -1) { #ifdef F_OFD_SETLK - int fd; int ret; struct flock fl = { .l_whence = SEEK_SET, @@ -200,17 +200,7 @@ static void qemu_probe_lock_ops(void) .l_type = F_WRLCK, }; - fd = open("/dev/null", O_RDWR); - if (fd < 0) { - fprintf(stderr, - "Failed to open /dev/null for OFD lock probing: %s\n", - strerror(errno)); - fcntl_op_setlk = F_SETLK; - fcntl_op_getlk = F_GETLK; - return; - } ret = fcntl(fd, F_OFD_GETLK, &fl); - close(fd); if (!ret) { fcntl_op_setlk = F_OFD_SETLK; fcntl_op_getlk = F_OFD_GETLK; @@ -225,9 +215,30 @@ static void qemu_probe_lock_ops(void) } } -bool qemu_has_ofd_lock(void) +static void qemu_probe_lock_ops(const char *filename) +{ + int fd; + if (filename) { + fd = open(filename, O_RDWR); + } else { + fd = open(null_device, O_RDONLY); + } + if (fd < 0) { + fprintf(stderr, + "Failed to open %s for OFD lock probing: %s\n", + filename ? filename : null_device, + strerror(errno)); + fcntl_op_setlk = F_SETLK; + fcntl_op_getlk = F_GETLK; + return; + } + qemu_probe_lock_ops_fd(fd); + close(fd); +} + +bool qemu_has_ofd_lock(const char *filename) { - qemu_probe_lock_ops(); + qemu_probe_lock_ops(filename); #ifdef F_OFD_SETLK return fcntl_op_setlk == F_OFD_SETLK; #else @@ -244,7 +255,7 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type) .l_len = len, .l_type = fl_type, }; - qemu_probe_lock_ops(); + qemu_probe_lock_ops_fd(fd); do { ret = fcntl(fd, fcntl_op_setlk, &fl); } while (ret == -1 && errno == EINTR); @@ -270,7 +281,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive) .l_len = len, .l_type = exclusive ? F_WRLCK : F_RDLCK, }; - qemu_probe_lock_ops(); + qemu_probe_lock_ops_fd(fd); ret = fcntl(fd, fcntl_op_getlk, &fl); if (ret == -1) { return -errno;
This patch addresses this issue: When accessing a volume on an NFS filesystem without supporting the file lock, tools, like qemu-img, will complain "Failed to lock byte 100". In the original code, the qemu_has_ofd_lock will test the lock on the "/dev/null" pseudo-file. Actually, the file.locking is per-drive property, which depends on the underlay filesystem. In this patch, make the 'qemu_has_ofd_lock' with a filename be more generic and reasonable. Signed-off-by: Li Feng <fengli@smartx.com> --- block/file-posix.c | 32 +++++++++++++++------------- include/qemu/osdep.h | 2 +- tests/test-image-locking.c | 2 +- util/osdep.c | 43 ++++++++++++++++++++++++-------------- 4 files changed, 47 insertions(+), 32 deletions(-)