Re: [dm-devel] [PATCH 1/1] dm raid: fix compat_features validation

[Heinz Mauelshagen]

On 10/11/2016 07:44 PM, Mike Snitzer wrote:

On Tue, Oct 11 2016 at 11:44am -0400,
Heinz Mauelshagen  wrote:



On 10/11/2016 05:38 PM, Andy Whitcroft wrote:

On Tue, Oct 11, 2016 at 05:04:34PM +0200, Heinz Mauelshagen wrote:

Andy,

good catch.

We should rather check for  V190 support only in case any
compat feature flags are actually set.

{
+   if (le32_to_cpu(sb->compat_features) &&
+   le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190)
{
 rs->ti->error = "Unable to assemble array: Unknown flag(s)
in compatible feature flags";
 return -EINVAL;
 }

If the feature flags are single bit combinations then I believe the
below does check exactly that.  Checking for no 1s outside of the
expected features, caring not for the value of the valid bits:

+ if (le32_to_cpu(sb->compat_features) & ~(FEATURE_FLAG_SUPPORTS_V190)) {

with the possibilty to or in additional feature bits as they are added.

Thanks,
I prefer this to be easier readable.

Readable or not, the code with the != is _not_ future-proof.  Whereas
Andy's solution is.  If/when a new compat feature comes along then
FEATURE_FLAG_SUPPORTS_V190 would be replaced to be a macro that ORs all
the new compat features together (e.g. FEATURE_FLAG_COMPAT).  E.g. how
dm-thin-metadata.c:__check_incompat_features() does.


If we'll have to introduce more feature flags in the future (e.g. for 
clustered raid1

support), this is going to be based on the test_bit() API for consistency
with any other flag processing we do in the target.

Heinz


We can go with the != code for now, since any future changes would
likely cause this test to be changed.  Or we could fix it now _for
real_.

Mike


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

[dm-devel] [PATCH 2/2] multipathd: Avoid that a deadlock is triggered sporadically during shutdown

[Bart Van Assche]

pthread_cond_wait() is a thread cancellation point. If a thread that
is waiting in pthread_cond_wait() is canceled it is possible that the
mutex is re-acquired before the first cancellation cleanup handler
is called. In this case the cleanup handler is uevq_stop() and that
function locks uevq_lock. Avoid that calling uevq_stop() results in
a deadlock due to an attempt to lock a non-recursive mutex recursively.

Signed-off-by: Bart Van Assche 
---
 libmultipath/uevent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 6247898..85fd2fb 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -52,7 +52,7 @@ typedef int (uev_trigger)(struct uevent *, void * 
trigger_data);
 
 pthread_t uevq_thr;
 LIST_HEAD(uevq);
-pthread_mutex_t uevq_lock = PTHREAD_MUTEX_INITIALIZER;
+pthread_mutex_t uevq_lock = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
 pthread_mutex_t *uevq_lockp = _lock;
 pthread_cond_t uev_cond = PTHREAD_COND_INITIALIZER;
 pthread_cond_t *uev_condp = _cond;
-- 
2.10.0

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

[dm-devel] [PATCH 0/2] Two multipath-tools bug fixes

[Bart Van Assche]

Hello Christophe,

Please consider the two patches in this series for inclusion in the 
multipath-tools repository. The first patch fixes a recently introduced 
bug and the second patch fixes a longstanding bug.


Thanks,

Bart.

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

Re: [dm-devel] dm-mpath: Handling SCSI-3 PR RELEASE in multi-controller environment

[Christoph Hellwig]

Hi Yibin,

>>> - For reserve/query/preempt/clear, we should return success once an
>>>  iteration returns successfully.
>> That's what the dm_grab_bdev_for_ioctl path does.
>
> If I understand correctly, dm_grab_bdev_for_ioctl() select a working path, 
> and
> pr_*() uses that path to do the actually work.
>
> This works for reserve/query/preempt/clear, but it may not work for 
> release.

You're right - if we had a path mapping change inbetween we might
have a problem with the unregister.  That beeing said we have an even
worse problem if the path we registered on disappeared permanently,
so we'll probably need a slightly more complex loop than the one
we have right now.

> I meant the API that callers from above block layer can use - They can not 
> call
> dm_pr_*() directly. So adding blkdev_pr_ops_{register/reserve/etc}() would 
> be
> great.

Take a look at the pNFS SCSI layout driver under fs/nfs/blocklayout
which is the currently existing in-kernel user of the API, it just
calls the block device methods directly.

What additional work would you place in the ops?  Also can you please
send me a link to your user of the API?

>
>>> 5. Support for multiple targets devices.
>>> An md device might have multiple targets. Current implementation only 
>>> supports
>>> single target device.
>> That's because it is so far only intended for dm-multipath, which always
>> uses as single target.  I'm not against multi-target support, but we'll
>> need a detailed explanation of the use case.
>
> OK. Fair enough. That's rather a "good-to-have" than a "must-have".

As said I'm fine with adding as an enhancement, especially if you submit
an in-kernel user for it, but a useful opensource application also
would be more than enough of a justification

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

Re: [dm-devel] [PATCH] hwe_regmatch: match error

[Hannes Reinecke]

On 10/14/2016 04:03 AM, huang.we...@zte.com.cn wrote:
> From: "wei.huang" 
> 
> Problem:
> when we configure a device like vendor, product, revision all null in 
> multipath.conf, hwe_regmatch always return 0.
> 
> Reasons:
> \!hwe2->vendor, \!hwe2->product and \!hwe2->revision are all true.
> 
> Signed-off-by: wei.huang 
> ---
>  libmultipath/config.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index a48b8af..d99cd75 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -80,7 +80,8 @@ hwe_regmatch (struct hwentry *hwe1, struct hwentry *hwe2)
>   regcomp(, hwe1->revision, REG_EXTENDED|REG_NOSUB))
>   goto out_pre;
>  
> - if ((!hwe1->vendor || !hwe2->vendor ||
> + if ((hwe2->vendor || hwe2->product || hwe2->revision) &&
> + (!hwe1->vendor || !hwe2->vendor ||
>!regexec(, hwe2->vendor, 0, NULL, 0)) &&
>   (!hwe1->product || !hwe2->product ||
>!regexec(, hwe2->product, 0, NULL, 0)) &&
> 
Good point.

Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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