Re: [Qemu-devel] [PATCH] osdep: Retry SETLK upon EINTR

[Fam Zheng <famz@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