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
next prev parent reply other threads:[~2018-10-05 19:07 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-21 23:05 [PATCH v3 00/19] Misc Multipath patches Benjamin Marzinski
2018-09-21 23:05 ` [PATCH v3 01/19] libmultipath: fix tur checker timeout Benjamin Marzinski
2018-10-01 19:51 ` Martin Wilck
2018-10-04 16:31 ` Benjamin Marzinski
2018-10-05 10:11 ` Martin Wilck
2018-10-05 17:02 ` Benjamin Marzinski
2018-10-05 19:23 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 02/19] libmultipath: fix tur checker double locking Benjamin Marzinski
2018-10-01 20:09 ` Martin Wilck
2018-10-01 20:44 ` Martin Wilck
2018-10-04 16:47 ` Benjamin Marzinski
2018-10-04 16:45 ` Benjamin Marzinski
2018-10-05 10:25 ` Martin Wilck
2018-10-05 17:10 ` Benjamin Marzinski
2018-10-05 19:07 ` Martin Wilck [this message]
2018-09-21 23:05 ` [PATCH v3 03/19] libmultipath: fix tur memory misuse Benjamin Marzinski
2018-10-01 20:59 ` Martin Wilck
2018-10-02 7:48 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 04/19] libmultipath: cleanup tur locking Benjamin Marzinski
2018-10-01 21:08 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 05/19] libmultipath: fix tur checker timeout issue Benjamin Marzinski
2018-10-01 21:09 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 06/19] libmultipath: fix set_int error path Benjamin Marzinski
2018-10-01 21:17 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 07/19] libmultipath: fix length issues in get_vpd_sgio Benjamin Marzinski
2018-10-01 21:25 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 08/19] libmultipath: _install_keyword cleanup Benjamin Marzinski
2018-10-01 21:26 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 09/19] libmultipath: remove unused code Benjamin Marzinski
2018-10-01 21:28 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 10/19] libmultipath: fix memory issue in path_latency prio Benjamin Marzinski
2018-10-01 21:30 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 11/19] libmultipath: fix null dereference int alloc_path_group Benjamin Marzinski
2018-10-01 21:33 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 12/19] libmutipath: don't use malformed uevents Benjamin Marzinski
2018-10-01 21:31 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 13/19] multipath: fix max array size in print_cmd_valid Benjamin Marzinski
2018-10-01 21:35 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 14/19] multipathd: function return value tweaks Benjamin Marzinski
2018-10-01 21:37 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 15/19] multipathd: minor fixes Benjamin Marzinski
2018-10-01 21:38 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 16/19] multipathd: remove useless check and fix format Benjamin Marzinski
2018-10-01 21:40 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 17/19] multipathd: fix memory leak on error in configure Benjamin Marzinski
2018-10-01 21:42 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 18/19] libmultipath: Don't blank intialized paths Benjamin Marzinski
2018-10-01 22:00 ` Martin Wilck
2018-10-02 22:37 ` Martin Wilck
2018-10-05 19:38 ` Benjamin Marzinski
2018-10-08 9:41 ` Martin Wilck
2018-10-09 22:20 ` Benjamin Marzinski
2018-10-08 9:35 ` Martin Wilck
2018-09-21 23:05 ` [PATCH v3 19/19] libmultipath: Fixup updating paths Benjamin Marzinski
2018-10-01 22:30 ` Martin Wilck
2018-10-05 20:32 ` Benjamin Marzinski
2018-10-07 8:36 ` [PATCH v3 00/19] Misc Multipath patches Christophe Varoqui
2018-10-09 16:13 ` Benjamin Marzinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=da5d8b13f169cf85f8a4a35197585dde0eb104d6.camel@suse.com \
--to=mwilck@suse.com \
--cc=bart.vanassche@wdc.com \
--cc=bmarzins@redhat.com \
--cc=dm-devel@redhat.com \
--cc=mwilck+gmail@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.