* [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path @ 2018-05-14 12:13 ` Johannes Thumshirn 0 siblings, 0 replies; 18+ messages in thread From: Johannes Thumshirn @ 2018-05-14 12:13 UTC (permalink / raw) To: Keith Busch Cc: Sagi Grimberg, Christoph Hellwig, Linux NVMe Mailinglist, Linux Kernel Mailinglist, Hannes Reinecke, Johannes Thumshirn When running blktest's nvme/005 with a lockdep enabled kernel the test case fails due to the following lockdep splat in dmesg: [ 18.206166] ============================= [ 18.207286] WARNING: suspicious RCU usage [ 18.208417] 4.17.0-rc5 #881 Not tainted [ 18.209487] ----------------------------- [ 18.210612] drivers/nvme/host/nvme.h:457 suspicious rcu_dereference_check() usage! [ 18.213486] [ 18.213486] other info that might help us debug this: [ 18.213486] [ 18.214745] [ 18.214745] rcu_scheduler_active = 2, debug_locks = 1 [ 18.215798] 3 locks held by kworker/u32:5/1102: [ 18.216535] #0: (ptrval) ((wq_completion)"nvme-wq"){+.+.}, at: process_one_work+0x152/0x5c0 [ 18.217983] #1: (ptrval) ((work_completion)(&ctrl->scan_work)){+.+.}, at: process_one_work+0x152/0x5c0 [ 18.219584] #2: (ptrval) (&subsys->lock#2){+.+.}, at: nvme_ns_remove+0x43/0x1c0 [nvme_core] [ 18.221037] [ 18.221037] stack backtrace: [ 18.221721] CPU: 12 PID: 1102 Comm: kworker/u32:5 Not tainted 4.17.0-rc5 #881 [ 18.222830] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014 [ 18.224451] Workqueue: nvme-wq nvme_scan_work [nvme_core] [ 18.225308] Call Trace: [ 18.225704] dump_stack+0x78/0xb3 [ 18.226224] nvme_ns_remove+0x1a3/0x1c0 [nvme_core] [ 18.226975] nvme_validate_ns+0x87/0x850 [nvme_core] [ 18.227749] ? blk_queue_exit+0x69/0x110 [ 18.228358] ? blk_queue_exit+0x81/0x110 [ 18.228960] ? direct_make_request+0x1a0/0x1a0 [ 18.229649] nvme_scan_work+0x212/0x2d0 [nvme_core] [ 18.230411] process_one_work+0x1d8/0x5c0 [ 18.231037] ? process_one_work+0x152/0x5c0 [ 18.231705] worker_thread+0x45/0x3e0 [ 18.232282] kthread+0x101/0x140 [ 18.232788] ? process_one_work+0x5c0/0x5c0 The only caller of nvme_mpath_clear_current_path() is nvme_ns_remove() which holds the subsys lock so it's likely a false positive, but using rcu_dereference_protected() tells lockdep we're holding the lock. Fixes: 32acab3181c7 ("nvme: implement multipath access to nvme subsystems") Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- drivers/nvme/host/nvme.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 17d2f7cf3fed..ca034434ebb9 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -22,6 +22,7 @@ #include <linux/lightnvm.h> #include <linux/sed-opal.h> #include <linux/fault-inject.h> +#include <linux/rcupdate.h> extern unsigned int nvme_io_timeout; #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) { struct nvme_ns_head *head = ns->head; - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) + if (head && + ns == rcu_dereference_protected(head->current_path, + lockdep_is_held(&ns->ctrl->subsys->lock))) rcu_assign_pointer(head->current_path, NULL); } struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); -- 2.16.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path @ 2018-05-14 12:13 ` Johannes Thumshirn 0 siblings, 0 replies; 18+ messages in thread From: Johannes Thumshirn @ 2018-05-14 12:13 UTC (permalink / raw) When running blktest's nvme/005 with a lockdep enabled kernel the test case fails due to the following lockdep splat in dmesg: [ 18.206166] ============================= [ 18.207286] WARNING: suspicious RCU usage [ 18.208417] 4.17.0-rc5 #881 Not tainted [ 18.209487] ----------------------------- [ 18.210612] drivers/nvme/host/nvme.h:457 suspicious rcu_dereference_check() usage! [ 18.213486] [ 18.213486] other info that might help us debug this: [ 18.213486] [ 18.214745] [ 18.214745] rcu_scheduler_active = 2, debug_locks = 1 [ 18.215798] 3 locks held by kworker/u32:5/1102: [ 18.216535] #0: (ptrval) ((wq_completion)"nvme-wq"){+.+.}, at: process_one_work+0x152/0x5c0 [ 18.217983] #1: (ptrval) ((work_completion)(&ctrl->scan_work)){+.+.}, at: process_one_work+0x152/0x5c0 [ 18.219584] #2: (ptrval) (&subsys->lock#2){+.+.}, at: nvme_ns_remove+0x43/0x1c0 [nvme_core] [ 18.221037] [ 18.221037] stack backtrace: [ 18.221721] CPU: 12 PID: 1102 Comm: kworker/u32:5 Not tainted 4.17.0-rc5 #881 [ 18.222830] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014 [ 18.224451] Workqueue: nvme-wq nvme_scan_work [nvme_core] [ 18.225308] Call Trace: [ 18.225704] dump_stack+0x78/0xb3 [ 18.226224] nvme_ns_remove+0x1a3/0x1c0 [nvme_core] [ 18.226975] nvme_validate_ns+0x87/0x850 [nvme_core] [ 18.227749] ? blk_queue_exit+0x69/0x110 [ 18.228358] ? blk_queue_exit+0x81/0x110 [ 18.228960] ? direct_make_request+0x1a0/0x1a0 [ 18.229649] nvme_scan_work+0x212/0x2d0 [nvme_core] [ 18.230411] process_one_work+0x1d8/0x5c0 [ 18.231037] ? process_one_work+0x152/0x5c0 [ 18.231705] worker_thread+0x45/0x3e0 [ 18.232282] kthread+0x101/0x140 [ 18.232788] ? process_one_work+0x5c0/0x5c0 The only caller of nvme_mpath_clear_current_path() is nvme_ns_remove() which holds the subsys lock so it's likely a false positive, but using rcu_dereference_protected() tells lockdep we're holding the lock. Fixes: 32acab3181c7 ("nvme: implement multipath access to nvme subsystems") Signed-off-by: Johannes Thumshirn <jthumshirn at suse.de> --- drivers/nvme/host/nvme.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 17d2f7cf3fed..ca034434ebb9 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -22,6 +22,7 @@ #include <linux/lightnvm.h> #include <linux/sed-opal.h> #include <linux/fault-inject.h> +#include <linux/rcupdate.h> extern unsigned int nvme_io_timeout; #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) { struct nvme_ns_head *head = ns->head; - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) + if (head && + ns == rcu_dereference_protected(head->current_path, + lockdep_is_held(&ns->ctrl->subsys->lock))) rcu_assign_pointer(head->current_path, NULL); } struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); -- 2.16.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path 2018-05-14 12:13 ` Johannes Thumshirn @ 2018-05-14 12:42 ` Christoph Hellwig -1 siblings, 0 replies; 18+ messages in thread From: Christoph Hellwig @ 2018-05-14 12:42 UTC (permalink / raw) To: Johannes Thumshirn Cc: Keith Busch, Sagi Grimberg, Linux Kernel Mailinglist, Linux NVMe Mailinglist, Hannes Reinecke, Christoph Hellwig, paulmck > extern unsigned int nvme_io_timeout; > #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) > @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > { > struct nvme_ns_head *head = ns->head; > > - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) > + if (head && > + ns == rcu_dereference_protected(head->current_path, > + lockdep_is_held(&ns->ctrl->subsys->lock))) > rcu_assign_pointer(head->current_path, NULL); > } > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); We don't really dereference it at all in fact, but just check the pointers for equality. I wonder if there is a better way to do this, as my ANA patches add a caller without the lock (and withou SRU protection either now that I think of it) - for a pure pointer compare we really should not need any sort of protection. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path @ 2018-05-14 12:42 ` Christoph Hellwig 0 siblings, 0 replies; 18+ messages in thread From: Christoph Hellwig @ 2018-05-14 12:42 UTC (permalink / raw) > extern unsigned int nvme_io_timeout; > #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) > @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > { > struct nvme_ns_head *head = ns->head; > > - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) > + if (head && > + ns == rcu_dereference_protected(head->current_path, > + lockdep_is_held(&ns->ctrl->subsys->lock))) > rcu_assign_pointer(head->current_path, NULL); > } > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); We don't really dereference it at all in fact, but just check the pointers for equality. I wonder if there is a better way to do this, as my ANA patches add a caller without the lock (and withou SRU protection either now that I think of it) - for a pure pointer compare we really should not need any sort of protection. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path 2018-05-14 12:42 ` Christoph Hellwig @ 2018-05-14 12:57 ` Johannes Thumshirn -1 siblings, 0 replies; 18+ messages in thread From: Johannes Thumshirn @ 2018-05-14 12:57 UTC (permalink / raw) To: Christoph Hellwig Cc: Keith Busch, Sagi Grimberg, Linux Kernel Mailinglist, Linux NVMe Mailinglist, Hannes Reinecke, Christoph Hellwig, paulmck On Mon, May 14, 2018 at 05:42:30AM -0700, Christoph Hellwig wrote: > > extern unsigned int nvme_io_timeout; > > #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) > > @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > > { > > struct nvme_ns_head *head = ns->head; > > > > - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) > > + if (head && > > + ns == rcu_dereference_protected(head->current_path, > > + lockdep_is_held(&ns->ctrl->subsys->lock))) > > rcu_assign_pointer(head->current_path, NULL); > > } > > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); > > We don't really dereference it at all in fact, but just check the > pointers for equality. I wonder if there is a better way to do this, > as my ANA patches add a caller without the lock (and withou SRU > protection either now that I think of it) - for a pure pointer compare > we really should not need any sort of protection. Uff maybe, but are you sure a comparison of two pointer is always atomic (on all architectures)? Paul, can you shed some light on us mere mortal, whether the above rcu_dereference_protected() is needed or if a simple ns == head->current_path is sufficient. Thanks, Johannes -- Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path @ 2018-05-14 12:57 ` Johannes Thumshirn 0 siblings, 0 replies; 18+ messages in thread From: Johannes Thumshirn @ 2018-05-14 12:57 UTC (permalink / raw) On Mon, May 14, 2018@05:42:30AM -0700, Christoph Hellwig wrote: > > extern unsigned int nvme_io_timeout; > > #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) > > @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > > { > > struct nvme_ns_head *head = ns->head; > > > > - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) > > + if (head && > > + ns == rcu_dereference_protected(head->current_path, > > + lockdep_is_held(&ns->ctrl->subsys->lock))) > > rcu_assign_pointer(head->current_path, NULL); > > } > > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); > > We don't really dereference it at all in fact, but just check the > pointers for equality. I wonder if there is a better way to do this, > as my ANA patches add a caller without the lock (and withou SRU > protection either now that I think of it) - for a pure pointer compare > we really should not need any sort of protection. Uff maybe, but are you sure a comparison of two pointer is always atomic (on all architectures)? Paul, can you shed some light on us mere mortal, whether the above rcu_dereference_protected() is needed or if a simple ns == head->current_path is sufficient. Thanks, Johannes -- Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path 2018-05-14 12:57 ` Johannes Thumshirn @ 2018-05-14 13:38 ` Paul E. McKenney -1 siblings, 0 replies; 18+ messages in thread From: Paul E. McKenney @ 2018-05-14 13:38 UTC (permalink / raw) To: Johannes Thumshirn Cc: Christoph Hellwig, Keith Busch, Sagi Grimberg, Linux Kernel Mailinglist, Linux NVMe Mailinglist, Hannes Reinecke, Christoph Hellwig On Mon, May 14, 2018 at 02:57:25PM +0200, Johannes Thumshirn wrote: > On Mon, May 14, 2018 at 05:42:30AM -0700, Christoph Hellwig wrote: > > > extern unsigned int nvme_io_timeout; > > > #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) > > > @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > > > { > > > struct nvme_ns_head *head = ns->head; > > > > > > - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) > > > + if (head && > > > + ns == rcu_dereference_protected(head->current_path, > > > + lockdep_is_held(&ns->ctrl->subsys->lock))) > > > rcu_assign_pointer(head->current_path, NULL); > > > } > > > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); > > > > We don't really dereference it at all in fact, but just check the > > pointers for equality. I wonder if there is a better way to do this, > > as my ANA patches add a caller without the lock (and withou SRU > > protection either now that I think of it) - for a pure pointer compare > > we really should not need any sort of protection. > > Uff maybe, but are you sure a comparison of two pointer is always > atomic (on all architectures)? > > Paul, can you shed some light on us mere mortal, whether the above > rcu_dereference_protected() is needed or if a simple ns == > head->current_path is sufficient. One approach is the following: static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) { struct nvme_ns_head *head = ns->head; if (head && ns == rcu_access_pointer(head->current_path)) rcu_assign_pointer(head->current_path, NULL); } Without the rcu_access_pointer(), sparse (and thus the 0-day test robot) will complain that you are accessing an RCU-protected pointer without using RCU. However, rcu_access_pointer() won't ever give any lockdep splats about there being no RCU read-side critical section. You might still want rcu_dereference_protected() because it will yell at you if the lock is not held. Yes, the comparison will still be valid without the lock (at least at the exact moment when the load occurred), but the rcu_assign_pointer() might be a bit problematic if that lock is not held, right? But it is your guys' code, so I must defer to you for the intent. Thanx, Paul ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path @ 2018-05-14 13:38 ` Paul E. McKenney 0 siblings, 0 replies; 18+ messages in thread From: Paul E. McKenney @ 2018-05-14 13:38 UTC (permalink / raw) On Mon, May 14, 2018@02:57:25PM +0200, Johannes Thumshirn wrote: > On Mon, May 14, 2018@05:42:30AM -0700, Christoph Hellwig wrote: > > > extern unsigned int nvme_io_timeout; > > > #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) > > > @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > > > { > > > struct nvme_ns_head *head = ns->head; > > > > > > - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) > > > + if (head && > > > + ns == rcu_dereference_protected(head->current_path, > > > + lockdep_is_held(&ns->ctrl->subsys->lock))) > > > rcu_assign_pointer(head->current_path, NULL); > > > } > > > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); > > > > We don't really dereference it at all in fact, but just check the > > pointers for equality. I wonder if there is a better way to do this, > > as my ANA patches add a caller without the lock (and withou SRU > > protection either now that I think of it) - for a pure pointer compare > > we really should not need any sort of protection. > > Uff maybe, but are you sure a comparison of two pointer is always > atomic (on all architectures)? > > Paul, can you shed some light on us mere mortal, whether the above > rcu_dereference_protected() is needed or if a simple ns == > head->current_path is sufficient. One approach is the following: static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) { struct nvme_ns_head *head = ns->head; if (head && ns == rcu_access_pointer(head->current_path)) rcu_assign_pointer(head->current_path, NULL); } Without the rcu_access_pointer(), sparse (and thus the 0-day test robot) will complain that you are accessing an RCU-protected pointer without using RCU. However, rcu_access_pointer() won't ever give any lockdep splats about there being no RCU read-side critical section. You might still want rcu_dereference_protected() because it will yell at you if the lock is not held. Yes, the comparison will still be valid without the lock (at least at the exact moment when the load occurred), but the rcu_assign_pointer() might be a bit problematic if that lock is not held, right? But it is your guys' code, so I must defer to you for the intent. Thanx, Paul ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path 2018-05-14 13:38 ` Paul E. McKenney @ 2018-05-14 13:56 ` Johannes Thumshirn -1 siblings, 0 replies; 18+ messages in thread From: Johannes Thumshirn @ 2018-05-14 13:56 UTC (permalink / raw) To: Paul E. McKenney Cc: Keith Busch, Sagi Grimberg, Linux Kernel Mailinglist, Linux NVMe Mailinglist, Christoph Hellwig, Hannes Reinecke, Christoph Hellwig On Mon, May 14, 2018 at 06:38:49AM -0700, Paul E. McKenney wrote: > On Mon, May 14, 2018 at 02:57:25PM +0200, Johannes Thumshirn wrote: > > On Mon, May 14, 2018 at 05:42:30AM -0700, Christoph Hellwig wrote: > > > > extern unsigned int nvme_io_timeout; > > > > #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) > > > > @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > > > > { > > > > struct nvme_ns_head *head = ns->head; > > > > > > > > - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) > > > > + if (head && > > > > + ns == rcu_dereference_protected(head->current_path, > > > > + lockdep_is_held(&ns->ctrl->subsys->lock))) > > > > rcu_assign_pointer(head->current_path, NULL); > > > > } > > > > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); > > > > > > We don't really dereference it at all in fact, but just check the > > > pointers for equality. I wonder if there is a better way to do this, > > > as my ANA patches add a caller without the lock (and withou SRU > > > protection either now that I think of it) - for a pure pointer compare > > > we really should not need any sort of protection. > > > > Uff maybe, but are you sure a comparison of two pointer is always > > atomic (on all architectures)? > > > > Paul, can you shed some light on us mere mortal, whether the above > > rcu_dereference_protected() is needed or if a simple ns == > > head->current_path is sufficient. > > One approach is the following: > > static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > { > struct nvme_ns_head *head = ns->head; > > if (head && ns == rcu_access_pointer(head->current_path)) > rcu_assign_pointer(head->current_path, NULL); > } Yes that's what I have now as well, and it tests fine. > > Without the rcu_access_pointer(), sparse (and thus the 0-day test robot) > will complain that you are accessing an RCU-protected pointer without > using RCU. However, rcu_access_pointer() won't ever give any lockdep > splats about there being no RCU read-side critical section. > > You might still want rcu_dereference_protected() because it will yell > at you if the lock is not held. Yes, the comparison will still be valid > without the lock (at least at the exact moment when the load occurred), > but the rcu_assign_pointer() might be a bit problematic if that lock is > not held, right? > > But it is your guys' code, so I must defer to you for the intent. > > Thanx, Paul > > > _______________________________________________ > Linux-nvme mailing list > Linux-nvme@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-nvme -- Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path @ 2018-05-14 13:56 ` Johannes Thumshirn 0 siblings, 0 replies; 18+ messages in thread From: Johannes Thumshirn @ 2018-05-14 13:56 UTC (permalink / raw) On Mon, May 14, 2018@06:38:49AM -0700, Paul E. McKenney wrote: > On Mon, May 14, 2018@02:57:25PM +0200, Johannes Thumshirn wrote: > > On Mon, May 14, 2018@05:42:30AM -0700, Christoph Hellwig wrote: > > > > extern unsigned int nvme_io_timeout; > > > > #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) > > > > @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > > > > { > > > > struct nvme_ns_head *head = ns->head; > > > > > > > > - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) > > > > + if (head && > > > > + ns == rcu_dereference_protected(head->current_path, > > > > + lockdep_is_held(&ns->ctrl->subsys->lock))) > > > > rcu_assign_pointer(head->current_path, NULL); > > > > } > > > > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); > > > > > > We don't really dereference it at all in fact, but just check the > > > pointers for equality. I wonder if there is a better way to do this, > > > as my ANA patches add a caller without the lock (and withou SRU > > > protection either now that I think of it) - for a pure pointer compare > > > we really should not need any sort of protection. > > > > Uff maybe, but are you sure a comparison of two pointer is always > > atomic (on all architectures)? > > > > Paul, can you shed some light on us mere mortal, whether the above > > rcu_dereference_protected() is needed or if a simple ns == > > head->current_path is sufficient. > > One approach is the following: > > static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > { > struct nvme_ns_head *head = ns->head; > > if (head && ns == rcu_access_pointer(head->current_path)) > rcu_assign_pointer(head->current_path, NULL); > } Yes that's what I have now as well, and it tests fine. > > Without the rcu_access_pointer(), sparse (and thus the 0-day test robot) > will complain that you are accessing an RCU-protected pointer without > using RCU. However, rcu_access_pointer() won't ever give any lockdep > splats about there being no RCU read-side critical section. > > You might still want rcu_dereference_protected() because it will yell > at you if the lock is not held. Yes, the comparison will still be valid > without the lock (at least at the exact moment when the load occurred), > but the rcu_assign_pointer() might be a bit problematic if that lock is > not held, right? > > But it is your guys' code, so I must defer to you for the intent. > > Thanx, Paul > > > _______________________________________________ > Linux-nvme mailing list > Linux-nvme at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-nvme -- Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path 2018-05-14 13:56 ` Johannes Thumshirn @ 2018-05-14 16:08 ` Paul E. McKenney -1 siblings, 0 replies; 18+ messages in thread From: Paul E. McKenney @ 2018-05-14 16:08 UTC (permalink / raw) To: Johannes Thumshirn Cc: Keith Busch, Sagi Grimberg, Linux Kernel Mailinglist, Linux NVMe Mailinglist, Christoph Hellwig, Hannes Reinecke, Christoph Hellwig On Mon, May 14, 2018 at 03:56:22PM +0200, Johannes Thumshirn wrote: > On Mon, May 14, 2018 at 06:38:49AM -0700, Paul E. McKenney wrote: > > On Mon, May 14, 2018 at 02:57:25PM +0200, Johannes Thumshirn wrote: > > > On Mon, May 14, 2018 at 05:42:30AM -0700, Christoph Hellwig wrote: > > > > > extern unsigned int nvme_io_timeout; > > > > > #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) > > > > > @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > > > > > { > > > > > struct nvme_ns_head *head = ns->head; > > > > > > > > > > - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) > > > > > + if (head && > > > > > + ns == rcu_dereference_protected(head->current_path, > > > > > + lockdep_is_held(&ns->ctrl->subsys->lock))) > > > > > rcu_assign_pointer(head->current_path, NULL); > > > > > } > > > > > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); > > > > > > > > We don't really dereference it at all in fact, but just check the > > > > pointers for equality. I wonder if there is a better way to do this, > > > > as my ANA patches add a caller without the lock (and withou SRU > > > > protection either now that I think of it) - for a pure pointer compare > > > > we really should not need any sort of protection. > > > > > > Uff maybe, but are you sure a comparison of two pointer is always > > > atomic (on all architectures)? > > > > > > Paul, can you shed some light on us mere mortal, whether the above > > > rcu_dereference_protected() is needed or if a simple ns == > > > head->current_path is sufficient. > > > > One approach is the following: > > > > static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > > { > > struct nvme_ns_head *head = ns->head; > > > > if (head && ns == rcu_access_pointer(head->current_path)) > > rcu_assign_pointer(head->current_path, NULL); > > } > > Yes that's what I have now as well, and it tests fine. Very good! If it turns out to be useful, you can of course directly use lockdep_assert_held() to verify that the lock is held. Thanx, Paul > > Without the rcu_access_pointer(), sparse (and thus the 0-day test robot) > > will complain that you are accessing an RCU-protected pointer without > > using RCU. However, rcu_access_pointer() won't ever give any lockdep > > splats about there being no RCU read-side critical section. > > > > You might still want rcu_dereference_protected() because it will yell > > at you if the lock is not held. Yes, the comparison will still be valid > > without the lock (at least at the exact moment when the load occurred), > > but the rcu_assign_pointer() might be a bit problematic if that lock is > > not held, right? > > > > But it is your guys' code, so I must defer to you for the intent. > > > > Thanx, Paul > > > > > > _______________________________________________ > > Linux-nvme mailing list > > Linux-nvme@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-nvme > > -- > Johannes Thumshirn Storage > jthumshirn@suse.de +49 911 74053 689 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg) > Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path @ 2018-05-14 16:08 ` Paul E. McKenney 0 siblings, 0 replies; 18+ messages in thread From: Paul E. McKenney @ 2018-05-14 16:08 UTC (permalink / raw) On Mon, May 14, 2018@03:56:22PM +0200, Johannes Thumshirn wrote: > On Mon, May 14, 2018@06:38:49AM -0700, Paul E. McKenney wrote: > > On Mon, May 14, 2018@02:57:25PM +0200, Johannes Thumshirn wrote: > > > On Mon, May 14, 2018@05:42:30AM -0700, Christoph Hellwig wrote: > > > > > extern unsigned int nvme_io_timeout; > > > > > #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) > > > > > @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > > > > > { > > > > > struct nvme_ns_head *head = ns->head; > > > > > > > > > > - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) > > > > > + if (head && > > > > > + ns == rcu_dereference_protected(head->current_path, > > > > > + lockdep_is_held(&ns->ctrl->subsys->lock))) > > > > > rcu_assign_pointer(head->current_path, NULL); > > > > > } > > > > > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); > > > > > > > > We don't really dereference it at all in fact, but just check the > > > > pointers for equality. I wonder if there is a better way to do this, > > > > as my ANA patches add a caller without the lock (and withou SRU > > > > protection either now that I think of it) - for a pure pointer compare > > > > we really should not need any sort of protection. > > > > > > Uff maybe, but are you sure a comparison of two pointer is always > > > atomic (on all architectures)? > > > > > > Paul, can you shed some light on us mere mortal, whether the above > > > rcu_dereference_protected() is needed or if a simple ns == > > > head->current_path is sufficient. > > > > One approach is the following: > > > > static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > > { > > struct nvme_ns_head *head = ns->head; > > > > if (head && ns == rcu_access_pointer(head->current_path)) > > rcu_assign_pointer(head->current_path, NULL); > > } > > Yes that's what I have now as well, and it tests fine. Very good! If it turns out to be useful, you can of course directly use lockdep_assert_held() to verify that the lock is held. Thanx, Paul > > Without the rcu_access_pointer(), sparse (and thus the 0-day test robot) > > will complain that you are accessing an RCU-protected pointer without > > using RCU. However, rcu_access_pointer() won't ever give any lockdep > > splats about there being no RCU read-side critical section. > > > > You might still want rcu_dereference_protected() because it will yell > > at you if the lock is not held. Yes, the comparison will still be valid > > without the lock (at least at the exact moment when the load occurred), > > but the rcu_assign_pointer() might be a bit problematic if that lock is > > not held, right? > > > > But it is your guys' code, so I must defer to you for the intent. > > > > Thanx, Paul > > > > > > _______________________________________________ > > Linux-nvme mailing list > > Linux-nvme at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-nvme > > -- > Johannes Thumshirn Storage > jthumshirn at suse.de +49 911 74053 689 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg > GF: Felix Imend?rffer, Jane Smithard, Graham Norton > HRB 21284 (AG N?rnberg) > Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path 2018-05-14 12:42 ` Christoph Hellwig @ 2018-05-14 13:31 ` Paul E. McKenney -1 siblings, 0 replies; 18+ messages in thread From: Paul E. McKenney @ 2018-05-14 13:31 UTC (permalink / raw) To: Christoph Hellwig Cc: Johannes Thumshirn, Keith Busch, Sagi Grimberg, Linux Kernel Mailinglist, Linux NVMe Mailinglist, Hannes Reinecke, Christoph Hellwig On Mon, May 14, 2018 at 05:42:30AM -0700, Christoph Hellwig wrote: > > extern unsigned int nvme_io_timeout; > > #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) > > @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > > { > > struct nvme_ns_head *head = ns->head; > > > > - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) > > + if (head && > > + ns == rcu_dereference_protected(head->current_path, > > + lockdep_is_held(&ns->ctrl->subsys->lock))) > > rcu_assign_pointer(head->current_path, NULL); > > } > > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); > > We don't really dereference it at all in fact, but just check the > pointers for equality. I wonder if there is a better way to do this, > as my ANA patches add a caller without the lock (and withou SRU > protection either now that I think of it) - for a pure pointer compare > we really should not need any sort of protection. If you are just looking at the value of an RCU-protected pointer, then using rcu_access_pointer() will cause RCU to just read out the value and otherwise keeps its mouth shut. If you use rcu_access_pointer() and later dereference the value without protection, you will of course get what you deserve, good and hard. ;-) Thanx, Paul ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path @ 2018-05-14 13:31 ` Paul E. McKenney 0 siblings, 0 replies; 18+ messages in thread From: Paul E. McKenney @ 2018-05-14 13:31 UTC (permalink / raw) On Mon, May 14, 2018@05:42:30AM -0700, Christoph Hellwig wrote: > > extern unsigned int nvme_io_timeout; > > #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ) > > @@ -454,7 +455,9 @@ static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns) > > { > > struct nvme_ns_head *head = ns->head; > > > > - if (head && ns == srcu_dereference(head->current_path, &head->srcu)) > > + if (head && > > + ns == rcu_dereference_protected(head->current_path, > > + lockdep_is_held(&ns->ctrl->subsys->lock))) > > rcu_assign_pointer(head->current_path, NULL); > > } > > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); > > We don't really dereference it at all in fact, but just check the > pointers for equality. I wonder if there is a better way to do this, > as my ANA patches add a caller without the lock (and withou SRU > protection either now that I think of it) - for a pure pointer compare > we really should not need any sort of protection. If you are just looking at the value of an RCU-protected pointer, then using rcu_access_pointer() will cause RCU to just read out the value and otherwise keeps its mouth shut. If you use rcu_access_pointer() and later dereference the value without protection, you will of course get what you deserve, good and hard. ;-) Thanx, Paul ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path 2018-05-14 13:31 ` Paul E. McKenney @ 2018-05-14 13:34 ` Christoph Hellwig -1 siblings, 0 replies; 18+ messages in thread From: Christoph Hellwig @ 2018-05-14 13:34 UTC (permalink / raw) To: Paul E. McKenney Cc: Christoph Hellwig, Johannes Thumshirn, Keith Busch, Sagi Grimberg, Linux Kernel Mailinglist, Linux NVMe Mailinglist, Hannes Reinecke, Christoph Hellwig On Mon, May 14, 2018 at 06:31:05AM -0700, Paul E. McKenney wrote: > > > + if (head && > > > + ns == rcu_dereference_protected(head->current_path, > > > + lockdep_is_held(&ns->ctrl->subsys->lock))) > > > rcu_assign_pointer(head->current_path, NULL); > > > } > > > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); > > > > We don't really dereference it at all in fact, but just check the > > pointers for equality. I wonder if there is a better way to do this, > > as my ANA patches add a caller without the lock (and withou SRU > > protection either now that I think of it) - for a pure pointer compare > > we really should not need any sort of protection. > > If you are just looking at the value of an RCU-protected pointer, then > using rcu_access_pointer() will cause RCU to just read out the value > and otherwise keeps its mouth shut. That is exactly the function I was looking for. And given that srcu and rcu use the same annotations I should have through of being able to use it of course. As you see above we only use the return value to do a comparison, so we should be perfectly fine. Johannes, can you respin the patch to use rcu_access_pointer? ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path @ 2018-05-14 13:34 ` Christoph Hellwig 0 siblings, 0 replies; 18+ messages in thread From: Christoph Hellwig @ 2018-05-14 13:34 UTC (permalink / raw) On Mon, May 14, 2018@06:31:05AM -0700, Paul E. McKenney wrote: > > > + if (head && > > > + ns == rcu_dereference_protected(head->current_path, > > > + lockdep_is_held(&ns->ctrl->subsys->lock))) > > > rcu_assign_pointer(head->current_path, NULL); > > > } > > > struct nvme_ns *nvme_find_path(struct nvme_ns_head *head); > > > > We don't really dereference it at all in fact, but just check the > > pointers for equality. I wonder if there is a better way to do this, > > as my ANA patches add a caller without the lock (and withou SRU > > protection either now that I think of it) - for a pure pointer compare > > we really should not need any sort of protection. > > If you are just looking at the value of an RCU-protected pointer, then > using rcu_access_pointer() will cause RCU to just read out the value > and otherwise keeps its mouth shut. That is exactly the function I was looking for. And given that srcu and rcu use the same annotations I should have through of being able to use it of course. As you see above we only use the return value to do a comparison, so we should be perfectly fine. Johannes, can you respin the patch to use rcu_access_pointer? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path 2018-05-14 13:31 ` Paul E. McKenney @ 2018-05-14 13:34 ` Johannes Thumshirn -1 siblings, 0 replies; 18+ messages in thread From: Johannes Thumshirn @ 2018-05-14 13:34 UTC (permalink / raw) To: Paul E. McKenney Cc: Christoph Hellwig, Keith Busch, Sagi Grimberg, Linux Kernel Mailinglist, Linux NVMe Mailinglist, Hannes Reinecke, Christoph Hellwig On Mon, May 14, 2018 at 06:31:05AM -0700, Paul E. McKenney wrote: > If you are just looking at the value of an RCU-protected pointer, then > using rcu_access_pointer() will cause RCU to just read out the value > and otherwise keeps its mouth shut. > > If you use rcu_access_pointer() and later dereference the value without > protection, you will of course get what you deserve, good and hard. ;-) Thanks Paul. Christoph, I'll be sending the v2 probably tomorrow as I have more lockdep fixes for nvme in the pipe and I'll send them out as a complete series. -- Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path @ 2018-05-14 13:34 ` Johannes Thumshirn 0 siblings, 0 replies; 18+ messages in thread From: Johannes Thumshirn @ 2018-05-14 13:34 UTC (permalink / raw) On Mon, May 14, 2018@06:31:05AM -0700, Paul E. McKenney wrote: > If you are just looking at the value of an RCU-protected pointer, then > using rcu_access_pointer() will cause RCU to just read out the value > and otherwise keeps its mouth shut. > > If you use rcu_access_pointer() and later dereference the value without > protection, you will of course get what you deserve, good and hard. ;-) Thanks Paul. Christoph, I'll be sending the v2 probably tomorrow as I have more lockdep fixes for nvme in the pipe and I'll send them out as a complete series. -- Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-05-14 16:08 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-14 12:13 [PATCH] nvme: fix lockdep warning in nvme_mpath_clear_current_path Johannes Thumshirn 2018-05-14 12:13 ` Johannes Thumshirn 2018-05-14 12:42 ` Christoph Hellwig 2018-05-14 12:42 ` Christoph Hellwig 2018-05-14 12:57 ` Johannes Thumshirn 2018-05-14 12:57 ` Johannes Thumshirn 2018-05-14 13:38 ` Paul E. McKenney 2018-05-14 13:38 ` Paul E. McKenney 2018-05-14 13:56 ` Johannes Thumshirn 2018-05-14 13:56 ` Johannes Thumshirn 2018-05-14 16:08 ` Paul E. McKenney 2018-05-14 16:08 ` Paul E. McKenney 2018-05-14 13:31 ` Paul E. McKenney 2018-05-14 13:31 ` Paul E. McKenney 2018-05-14 13:34 ` Christoph Hellwig 2018-05-14 13:34 ` Christoph Hellwig 2018-05-14 13:34 ` Johannes Thumshirn 2018-05-14 13:34 ` Johannes Thumshirn
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.