| Message ID | 20171226065300.9890-1-famz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/26/2017 12:53 AM, Fam Zheng wrote: > We could hit lock failure if there is a signal that makes fcntl return > -1 and errno set to EINTR. In this case we should retry. Did you hit this in practice? In 'man fcntl' on my Fedora 27 box, the DESCRIPTION section only mentions EINTR as possible for F_[OFD_]SETLKW, but we don't appear to be using that one (just SETLK and GETLK). On the other hand, the ERRORS section of the same document mentions: EINTR cmd is F_SETLKW or F_OFD_SETLKW and the operation was inter‐ rupted by a signal; see signal(7). EINTR cmd is F_GETLK, F_SETLK, F_OFD_GETLK, or F_OFD_SETLK, and the operation was interrupted by a signal before the lock was checked or acquired. Most likely when locking a remote file (e.g., locking over NFS), but can sometimes happen locally. (I hate it when information differs between two places in the same document, especially if I only read the first place) > > Cc: qemu-stable@nongnu.org > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > util/osdep.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/util/osdep.c b/util/osdep.c > index 1231f9f876..a73de0e1ba 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -244,7 +244,9 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type) > .l_type = fl_type, > }; > qemu_probe_lock_ops(); > - ret = fcntl(fd, fcntl_op_setlk, &fl); > + do { > + ret = fcntl(fd, fcntl_op_setlk, &fl); > + } while (ret == -1 && errno == EINTR); The change makes sense from a maintenance point of view, whether or not you hit it in practice. Reviewed-by: Eric Blake <eblake@redhat.com>
On Wed, 01/03 16:57, Eric Blake wrote: > On 12/26/2017 12:53 AM, Fam Zheng wrote: > > We could hit lock failure if there is a signal that makes fcntl return > > -1 and errno set to EINTR. In this case we should retry. > > Did you hit this in practice? In 'man fcntl' on my Fedora 27 box, the > DESCRIPTION section only mentions EINTR as possible for F_[OFD_]SETLKW, > but we don't appear to be using that one (just SETLK and GETLK). On the > other hand, the ERRORS section of the same document mentions: > > > EINTR cmd is F_SETLKW or F_OFD_SETLKW and the operation was > inter‐ > rupted by a signal; see signal(7). > > EINTR cmd is F_GETLK, F_SETLK, F_OFD_GETLK, or F_OFD_SETLK, > and the > operation was interrupted by a signal before the > lock was > checked or acquired. Most likely when locking a remote > file > (e.g., locking over NFS), but can sometimes happen locally. > > (I hate it when information differs between two places in the same > document, especially if I only read the first place) Yes, our QE found it when hammering qemu-img convert with SIGUSR1. So both SETLK and SETLKW can get EINTR. > > > > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > util/osdep.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/util/osdep.c b/util/osdep.c > > index 1231f9f876..a73de0e1ba 100644 > > --- a/util/osdep.c > > +++ b/util/osdep.c > > @@ -244,7 +244,9 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type) > > .l_type = fl_type, > > }; > > qemu_probe_lock_ops(); > > - ret = fcntl(fd, fcntl_op_setlk, &fl); > > + do { > > + ret = fcntl(fd, fcntl_op_setlk, &fl); > > + } while (ret == -1 && errno == EINTR); > > The change makes sense from a maintenance point of view, whether or not > you hit it in practice. Thank you for reviewing! Fam
Am 26.12.2017 um 07:53 hat Fam Zheng geschrieben: > We could hit lock failure if there is a signal that makes fcntl return > -1 and errno set to EINTR. In this case we should retry. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Fam Zheng <famz@redhat.com> Thanks, applied to the block branch. Kevin
diff --git a/util/osdep.c b/util/osdep.c index 1231f9f876..a73de0e1ba 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -244,7 +244,9 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type) .l_type = fl_type, }; qemu_probe_lock_ops(); - ret = fcntl(fd, fcntl_op_setlk, &fl); + do { + ret = fcntl(fd, fcntl_op_setlk, &fl); + } while (ret == -1 && errno == EINTR); return ret == -1 ? -errno : 0; }
We could hit lock failure if there is a signal that makes fcntl return -1 and errno set to EINTR. In this case we should retry. Cc: qemu-stable@nongnu.org Signed-off-by: Fam Zheng <famz@redhat.com> --- util/osdep.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)