Re: [PATCH v3 02/19] libmultipath: fix tur checker double locking

From: Martin Wilck <mwilck@suse.com>
To: Benjamin Marzinski <bmarzins@redhat.com>, mwilck+gmail@suse.de
Cc: Bart Van Assche <bart.vanassche@wdc.com>,
	device-mapper development <dm-devel@redhat.com>
Subject: Re: [PATCH v3 02/19] libmultipath: fix tur checker double locking
Date: Fri, 05 Oct 2018 21:07:54 +0200	[thread overview]
Message-ID: <da5d8b13f169cf85f8a4a35197585dde0eb104d6.camel@suse.com> (raw)
In-Reply-To: <20181005171054.GE3172@octiron.msp.redhat.com>

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@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel