* [patch] btt: fix uninitialized err_lock
@ 2017-12-13 21:33 Jeff Moyer
2017-12-13 21:38 ` Verma, Vishal L
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Moyer @ 2017-12-13 21:33 UTC (permalink / raw)
To: dan.j.williams, vishal.l.verma; +Cc: linux-nvdimm
Hi,
When a sector mode namespace is initially created, the arena's err_lock
is not initialized. If, on the other hand, the namespace already
exists, the mutex is initialized. To fix the issue, I moved the mutex
initialization into the arena_alloc, which is called by both
discover_arenas and create_arenas.
This was discovered on an older kernel where mutex_trylock checks the
count to determine whether the lock is held. Because the data structure
is kzalloc-d, that count was 0 (held), and I/O to the device would hang
forever waiting for the lock to be released (see btt_write_pg, for
example). Current kernels have a different mutex implementation that
checks for a non-null owner, and so this doesn't show up as a problem.
If that lock were ever contended, it might cause issues, but you'd have
to be really unlucky, I think.
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index e949e33..5860f99 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -630,6 +630,7 @@ static struct arena_info *alloc_arena(struct btt *btt, size_t size,
return NULL;
arena->nd_btt = btt->nd_btt;
arena->sector_size = btt->sector_size;
+ mutex_init(&arena->err_lock);
if (!size)
return arena;
@@ -758,7 +759,6 @@ static int discover_arenas(struct btt *btt)
arena->external_lba_start = cur_nlba;
parse_arena_meta(arena, super, cur_off);
- mutex_init(&arena->err_lock);
ret = btt_freelist_init(arena);
if (ret)
goto out;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [patch] btt: fix uninitialized err_lock
2017-12-13 21:33 [patch] btt: fix uninitialized err_lock Jeff Moyer
@ 2017-12-13 21:38 ` Verma, Vishal L
2018-01-15 18:16 ` Jeff Moyer
0 siblings, 1 reply; 3+ messages in thread
From: Verma, Vishal L @ 2017-12-13 21:38 UTC (permalink / raw)
To: Williams, Dan J, jmoyer; +Cc: linux-nvdimm
On Wed, 2017-12-13 at 16:33 -0500, Jeff Moyer wrote:
> Hi,
>
> When a sector mode namespace is initially created, the arena's
> err_lock
> is not initialized. If, on the other hand, the namespace already
> exists, the mutex is initialized. To fix the issue, I moved the
> mutex
> initialization into the arena_alloc, which is called by both
> discover_arenas and create_arenas.
>
> This was discovered on an older kernel where mutex_trylock checks the
> count to determine whether the lock is held. Because the data
> structure
> is kzalloc-d, that count was 0 (held), and I/O to the device would
> hang
> forever waiting for the lock to be released (see btt_write_pg, for
> example). Current kernels have a different mutex implementation that
> checks for a non-null owner, and so this doesn't show up as a
> problem.
> If that lock were ever contended, it might cause issues, but you'd
> have
> to be really unlucky, I think.
>
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
Ah, good find - the fix looks good to me.
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
>
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index e949e33..5860f99 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -630,6 +630,7 @@ static struct arena_info *alloc_arena(struct btt
> *btt, size_t size,
> return NULL;
> arena->nd_btt = btt->nd_btt;
> arena->sector_size = btt->sector_size;
> + mutex_init(&arena->err_lock);
>
> if (!size)
> return arena;
> @@ -758,7 +759,6 @@ static int discover_arenas(struct btt *btt)
> arena->external_lba_start = cur_nlba;
> parse_arena_meta(arena, super, cur_off);
>
> - mutex_init(&arena->err_lock);
> ret = btt_freelist_init(arena);
> if (ret)
> goto out;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch] btt: fix uninitialized err_lock
2017-12-13 21:38 ` Verma, Vishal L
@ 2018-01-15 18:16 ` Jeff Moyer
0 siblings, 0 replies; 3+ messages in thread
From: Jeff Moyer @ 2018-01-15 18:16 UTC (permalink / raw)
To: Verma, Vishal L; +Cc: linux-nvdimm
Dan,
Any chance you can pull this one in? (I know you've been busy.)
-Jeff
"Verma, Vishal L" <vishal.l.verma@intel.com> writes:
> On Wed, 2017-12-13 at 16:33 -0500, Jeff Moyer wrote:
>> Hi,
>>
>> When a sector mode namespace is initially created, the arena's
>> err_lock
>> is not initialized. If, on the other hand, the namespace already
>> exists, the mutex is initialized. To fix the issue, I moved the
>> mutex
>> initialization into the arena_alloc, which is called by both
>> discover_arenas and create_arenas.
>>
>> This was discovered on an older kernel where mutex_trylock checks the
>> count to determine whether the lock is held. Because the data
>> structure
>> is kzalloc-d, that count was 0 (held), and I/O to the device would
>> hang
>> forever waiting for the lock to be released (see btt_write_pg, for
>> example). Current kernels have a different mutex implementation that
>> checks for a non-null owner, and so this doesn't show up as a
>> problem.
>> If that lock were ever contended, it might cause issues, but you'd
>> have
>> to be really unlucky, I think.
>>
>> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
>
> Ah, good find - the fix looks good to me.
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
>
>>
>> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
>> index e949e33..5860f99 100644
>> --- a/drivers/nvdimm/btt.c
>> +++ b/drivers/nvdimm/btt.c
>> @@ -630,6 +630,7 @@ static struct arena_info *alloc_arena(struct btt
>> *btt, size_t size,
>> return NULL;
>> arena->nd_btt = btt->nd_btt;
>> arena->sector_size = btt->sector_size;
>> + mutex_init(&arena->err_lock);
>>
>> if (!size)
>> return arena;
>> @@ -758,7 +759,6 @@ static int discover_arenas(struct btt *btt)
>> arena->external_lba_start = cur_nlba;
>> parse_arena_meta(arena, super, cur_off);
>>
>> - mutex_init(&arena->err_lock);
>> ret = btt_freelist_init(arena);
>> if (ret)
>> goto out;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-01-15 18:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-13 21:33 [patch] btt: fix uninitialized err_lock Jeff Moyer
2017-12-13 21:38 ` Verma, Vishal L
2018-01-15 18:16 ` Jeff Moyer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).