Hi, Daniel
Thanks for your reply.
I have just ended my trip, sorry for my late response.
I will send out the v4.
Daniel P. Berrangé <berra...@redhat.com> 于2020年12月11日周五 上午12:55写道：
>
> On Fri, Dec 11, 2020 at 12:38:19AM +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".
> >
> > Add a new function 'qemu_has_file_lock' to detect if the filesystem
> > supports locks
> > or not.
> > And when the drive is auto mode, use the 'qemu_has_file_lock' to set the
> > toggle.
> >
> > Signed-off-by: Li Feng <fen...@smartx.com>
> > ---
> > v3: don't call the qemu_has_ofd_lock, use a new function instead.
> > v2: remove the refactoring.
> > ---
> > block/file-posix.c | 30 +++++++++++++++++-------------
> > include/qemu/osdep.h | 1 +
> > util/osdep.c | 29 +++++++++++++++++++++++++++++
> > 3 files changed, 47 insertions(+), 13 deletions(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 806764f7e3..48f9a32de2 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -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_file_lock(filename);
>
> This is not good - it causes us to always use locks by default, where
> as previously we only used them if OFD was available. It neds to test
> both here, except opening + closing filename to test for fnctl support
> risks releasing any locks QEMU already holds on filename if OFD is not
> supported.
Yes, check the qemu_has_ofd_lock and qemu_has_file_lock both, and
set the use_lock to false when the os supports the OFD lock, but the
filesystem doesn't support.
>
> > 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_file_lock(file_opts->filename);
>
> This cause QEMU to open and close filename. If another thread
> already had filename open, and OFD is not support, we've just
> lock the locks we held. We need to use 'fd' which is already
> open.
Acked.
>
> > + 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 */
>
>
> > +bool qemu_has_file_lock(const char *filename)
>
> IMO thisshould just accept a pre-opened 'int fd'
Acked.
>
> > +{
> > +#ifdef F_OFD_SETLK
> > + int cmd = F_OFD_GETLK;
> > +#else
> > + int cmd = F_GETLK;
> > +#endif
> > + int fd;
> > + int ret;
> > + struct flock fl = {
> > + .l_whence = SEEK_SET,
> > + .l_start = 0,
> > + .l_len = 0,
> > + .l_type = F_WRLCK,
> > + };
> > +
> > + fd = open(filename, O_RDWR);
> > + if (fd < 0) {
> > + fprintf(stderr,
> > + "Failed to open %s for OFD lock probing: %s\n",
> > + filename,
> > + strerror(errno));
> > + return false;
> > + }
> > + ret = fcntl(fd, cmd, &fl);
> > + close(fd);
> > + return ret == 0;
> > +}
> > +
> > bool qemu_has_ofd_lock(void)
> > {
> > qemu_probe_lock_ops();
>
> 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 :|
>
