[dm-devel] [PATCH v3 02/19] libmultipath: fix tur checker double locking

[Martin Wilck]

On Fri, 2018-10-05 at 12:10 -0500,  Benjamin Marzinski  wrote:
> On Fri, Oct 05, 2018 at 12:25:15PM +0200, Martin Wilck wrote:
> > On Thu, 2018-10-04 at 11:45 -0500,  Benjamin Marzinski  wrote:
> > > On Mon, Oct 01, 2018 at 10:09:41PM +0200, Martin Wilck wrote:
> > > > On Fri, 2018-09-21 at 18:05 -0500, Benjamin Marzinski wrote:
> > > > > tur_devt() locks ct->lock. However, it is ocassionally called
> > > > > while
> > > > > ct->lock is already locked. In reality, there is no reason
> > > > > why we
> > > > > need
> > > > > to lock all the accesses to ct->devt. The tur checker only
> > > > > needs
> > > > > to
> > > > > write to this variable one time, when it first gets the file
> > > > > descripter
> > > > > that it is checking.  It also never uses ct->devt directly.
> > > > > Instead,
> > > > > it
> > > > > always graps the major and minor, and turns them into a
> > > > > string.
> > > > > This
> > > > > patch changes ct->devt into that string, and sets it in
> > > > > libcheck_init()
> > > > > when it is first initializing the checker context. After
> > > > > that,
> > > > > ct-
> > > > > > devt
> > > > > 
> > > > > is only ever read.
> > > > 
> > > > I like the lock removal a lot, but not so much the conversion
> > > > into
> > > > a
> > > > string. Why not keep the dev_t? 
> > > 
> > > Because we will simply convert it into a string every time we use
> > > it,
> > > instead of doing the work one time. It's 24 more bytes in the
> > > tur_checker_context, but the code is easier to read, and we're
> > > not
> > > doing
> > > the same work again and again.
> > 
> > Well, IMO snprintf(buf, N, "%d:%d", major(x), minor(x)) is not
> > _that_
> > much work, and it looks a lot cleaner, to me at least.
> > 
> > But OK, I'm not insisting on this. So, if you re-post, I'm going to
> > ack
> > the patch.
> > 
> > My thought for long term is is this actually this: ct->dev_t is
> > only
> > needed for creating messages to be stored in the checker->message
> > string, which I don't like either.
> > 
> > Why don't we replace the "message" field with an "int msgid", and
> > add a 
> > 
> >      char* (*get_message)(int msgid)
> > 
> > to the checker API?
> 
> I would not have a problem with a patch that did this.

Let's get your series finalized, and possibly merged, first.

Martin

-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)