Re: [PATCH v2] file-posix: detect the lock using the real file
Ok, I see. I will split the detect lock without changing the global variable to a separate function. Don't call the qemu_has_ofd_lock. Thanks. Daniel P. Berrangé 于2020年12月10日周四 下午11:59写道: > > On Wed, Dec 09, 2020 at 12:54:48PM +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. > > > > In this patch, make the 'qemu_has_ofd_lock' with a filename be more > > generic and reasonable. > > > > Signed-off-by: Li Feng > > --- > > v2: remove the refactoring. > > --- > > block/file-posix.c | 32 ++-- > > include/qemu/osdep.h | 2 +- > > tests/test-image-locking.c | 2 +- > > util/osdep.c | 19 --- > > 4 files changed, 32 insertions(+), 23 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(, , NULL); > > > > -if (qemu_has_ofd_lock()) { > > +if (qemu_has_ofd_lock(NULL)) { > > g_test_add_func("/image-locking/basic", test_image_locking_basic); > >
Re: [PATCH v2] file-posix: detect the lock using the real file
On Wed, Dec 09, 2020 at 12:54:48PM +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. > > In this patch, make the 'qemu_has_ofd_lock' with a filename be more > generic and reasonable. > > Signed-off-by: Li Feng > --- > v2: remove the refactoring. > --- > block/file-posix.c | 32 ++-- > include/qemu/osdep.h | 2 +- > tests/test-image-locking.c | 2 +- > util/osdep.c | 19 --- > 4 files changed, 32 insertions(+), 23 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(, , 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..20119aa9ae 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -187,7 +187,7 @@ 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(const char *filename) > { > if (fcntl_op_setlk == -1) { >
[PATCH v2] file-posix: detect the lock using the real file
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 --- v2: remove the refactoring. --- block/file-posix.c | 32 ++-- include/qemu/osdep.h | 2 +- tests/test-image-locking.c | 2 +- util/osdep.c | 19 --- 4 files changed, 32 insertions(+), 23 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(, , 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..20119aa9ae 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -187,7 +187,7 @@ 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(const char *filename) { if (fcntl_op_setlk == -1) { #ifdef F_OFD_SETLK @@ -200,10 +200,15 @@ static void qemu_probe_lock_ops(void) .l_type = F_WRLCK, }; -fd = open("/dev/null", O_RDWR); +if (filename) { +fd = open(filename, O_RDWR); +} else { +fd = open("/dev/null", O_RDONLY); +}
[PATCH v2] file-posix: detect the lock using the real file
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 --- v2: remove the refactoring. --- block/file-posix.c | 32 ++-- include/qemu/osdep.h | 2 +- tests/test-image-locking.c | 2 +- util/osdep.c | 19 --- 4 files changed, 32 insertions(+), 23 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(, , 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..20119aa9ae 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -187,7 +187,7 @@ 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(const char *filename) { if (fcntl_op_setlk == -1) { #ifdef F_OFD_SETLK @@ -200,10 +200,15 @@ static void qemu_probe_lock_ops(void) .l_type = F_WRLCK, }; -fd = open("/dev/null", O_RDWR); +if (filename) { +fd = open(filename, O_RDWR); +} else { +fd = open("/dev/null", O_RDONLY); +}