Re: [PATCH] virtiofsd: vu_dispatch locking should never fail

[Stefan Hajnoczi]

On Wed, Feb 03, 2021 at 11:29:15AM -0500, Vivek Goyal wrote:
> On Wed, Feb 03, 2021 at 05:08:57PM +0100, Greg Kurz wrote:
> > On Wed, 3 Feb 2021 10:59:34 -0500
> > Vivek Goyal <vgoyal@redhat.com> wrote:
> > 
> > > On Fri, Jan 29, 2021 at 04:53:12PM +0100, Greg Kurz wrote:
> > > > pthread_rwlock_rdlock() and pthread_rwlock_wrlock() can fail if a
> > > > deadlock condition is detected or the current thread already owns
> > > > the lock. They can also fail, like pthread_rwlock_unlock(), if the
> > > > mutex wasn't properly initialized. None of these are ever expected
> > > > to happen with fv_VuDev::vu_dispatch_rwlock.
> > > > 
> > > > Some users already check the return value and assert, some others
> > > > don't. Introduce rdlock/wrlock/unlock wrappers that just do the
> > > > former and use them everywhere.
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > >  tools/virtiofsd/fuse_virtio.c | 42 +++++++++++++++++++++++------------
> > > >  1 file changed, 28 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/tools/virtiofsd/fuse_virtio.c 
> > > > b/tools/virtiofsd/fuse_virtio.c
> > > > index ddcefee4272f..7ea269c4b65d 100644
> > > > --- a/tools/virtiofsd/fuse_virtio.c
> > > > +++ b/tools/virtiofsd/fuse_virtio.c
> > > > @@ -187,6 +187,24 @@ static void copy_iov(struct iovec *src_iov, int 
> > > > src_count,
> > > >      }
> > > >  }
> > > >  
> > > > +/*
> > > > + * pthread_rwlock_rdlock() and pthread_rwlock_wrlock can fail if
> > > > + * a deadlock condition is detected or the current thread already
> > > > + * owns the lock. They can also fail, like pthread_rwlock_unlock(),
> > > > + * if the mutex wasn't properly initialized. None of these are ever
> > > > + * expected to happen.
> > > > + */
> > > > +#define VU_DISPATCH_LOCK_OP(op)                              \
> > > > +static inline void vu_dispatch_##op(struct fv_VuDev *vud)    \
> > > > +{                                                            \
> > > > +    int ret = pthread_rwlock_##op(&vud->vu_dispatch_rwlock); \
> > > > +    assert(ret == 0);                                        \
> > > > +}
> > > > +
> > > > +VU_DISPATCH_LOCK_OP(rdlock);
> > > > +VU_DISPATCH_LOCK_OP(wrlock);
> > > > +VU_DISPATCH_LOCK_OP(unlock);
> > > > +
> > > 
> > > I generally do not prefer using macros to define functions as searching
> > > to functions declarations/definitions becomes harder. But I see lot
> > > of people prefer that because they can reduce number of lines of code.
> > > 
> > 
> > Well, I must admit I hesitated since this doesn't gain much in
> > terms of LoC compared to the expanded version. I'm perfectly
> > fine with dropping the macro in my v2 if this looks better
> > to you.
> 
> If you are posting V2 anyway, so lets do it. Agreed, we are not saving
> many lines where so why to use macros to define functions.

Nice. I also prefer the open-coded version because ctags won't be able
to interpret the macros :).

Stefan

signature.asc
Description: PGP signature