Re: [Cluster-devel] [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Steven Rostedt]

On Tue, 19 Sep 2017 13:41:35 -0700
Christoph Hellwig  wrote:

> Call it blk_trace mutex and move it right next to the blk_trace
> structure:
> 
> ifdef CONFIG_BLK_DEV_IO_TRACE
> struct blk_trace*blk_trace;
>   struct mutexblk_trace_mutex;
> #endif
> 
> which makes it completely obvious to any read what you are protecting
> with it.

As a 1000ft away bystander, this appears to be the most logical
solution.

-- Steve

Re: [Cluster-devel] [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Christoph Hellwig]

On Tue, Sep 19, 2017 at 11:58:34AM -0400, Waiman Long wrote:
> I was trying not to add a new mutex to a structure just for blktrace as
> it is an optional feature that is enabled only if the
> CONFIG_BLK_DEV_IO_TRACE config option is defined and it will only need
> to be taken occasionally.

So?  Make the lock optional, too.

> As filesystem freeze looks orthogonal to blktrace and the mutex also
> looks to be used sparingly, I think it is a good match to overload it to
> control blktrace as well.

If the functionally is orthogonal that is a reason not to share a lock,
not to the contrary.

> I could modify the patch to use a mutex in the request_queue structure.
> The current sysfs_lock mutex has about 74 references. So I am not
> totally sure if it is safe to reuse. So the only option is to add
> something like

> 
> #ifdef CONFIG_BLK_DEV_IO_TRACE
> struct mutex blktrace_mutex;
> #endif
> 
> to the request_queue structure. That structure is large enough that
> adding a mutex won't increase the size much percentage-wise.

Call it blk_trace mutex and move it right next to the blk_trace
structure:

ifdef CONFIG_BLK_DEV_IO_TRACE
struct blk_trace*blk_trace;
struct mutexblk_trace_mutex;
#endif

which makes it completely obvious to any read what you are protecting
with it.

Re: [Cluster-devel] [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Waiman Long]

On 09/19/2017 10:38 AM, Christoph Hellwig wrote:
> On Tue, Sep 19, 2017 at 08:49:12AM -0400, Waiman Long wrote:
>> On 09/18/2017 08:01 PM, Christoph Hellwig wrote:
>>> Taking a look at this it seems like using a lock in struct block_device
>>> isn't the right thing to do anyway - all the action is on fields in
>>> struct blk_trace, so having a lock inside that would make a lot more
>>> sense.
>>>
>>> It would also help to document what exactly we're actually protecting.
>> I think I documented in the patch that the lock has to protect changes
>> in the blktrace structure as well as the allocation and destruction of
>> it. Because of that, it can't be put inside the blktrace structure. The
>> original code use the bd_mutex of the block_device structure. I just
>> change the code to use another bd_fsfreeze_mutex in the same structure.
> Either way it has absolutely nothing to do with struct block_device,
> given that struct blk_trace hangs off the request_queue.
>
> Reusing a mutex just because it happens to live in a structure also
> generally is a bad idea if the protected data is in no way related.

I was trying not to add a new mutex to a structure just for blktrace as
it is an optional feature that is enabled only if the
CONFIG_BLK_DEV_IO_TRACE config option is defined and it will only need
to be taken occasionally.

As filesystem freeze looks orthogonal to blktrace and the mutex also
looks to be used sparingly, I think it is a good match to overload it to
control blktrace as well.

I could modify the patch to use a mutex in the request_queue structure.
The current sysfs_lock mutex has about 74 references. So I am not
totally sure if it is safe to reuse. So the only option is to add
something like

#ifdef CONFIG_BLK_DEV_IO_TRACE
struct mutex blktrace_mutex;
#endif

to the request_queue structure. That structure is large enough that
adding a mutex won't increase the size much percentage-wise.

I would like to keep the current patch as is as I don't see any problem
with it. However, I can revise the patch as discussed above if you guys
prefer that alternative.

Cheers,
Longman

Re: [Cluster-devel] [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Christoph Hellwig]

On Tue, Sep 19, 2017 at 08:49:12AM -0400, Waiman Long wrote:
> On 09/18/2017 08:01 PM, Christoph Hellwig wrote:
> > Taking a look at this it seems like using a lock in struct block_device
> > isn't the right thing to do anyway - all the action is on fields in
> > struct blk_trace, so having a lock inside that would make a lot more
> > sense.
> >
> > It would also help to document what exactly we're actually protecting.
> 
> I think I documented in the patch that the lock has to protect changes
> in the blktrace structure as well as the allocation and destruction of
> it. Because of that, it can't be put inside the blktrace structure. The
> original code use the bd_mutex of the block_device structure. I just
> change the code to use another bd_fsfreeze_mutex in the same structure.

Either way it has absolutely nothing to do with struct block_device,
given that struct blk_trace hangs off the request_queue.

Reusing a mutex just because it happens to live in a structure also
generally is a bad idea if the protected data is in no way related.

Re: [Cluster-devel] [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Waiman Long]

On 09/18/2017 08:01 PM, Christoph Hellwig wrote:
> Taking a look at this it seems like using a lock in struct block_device
> isn't the right thing to do anyway - all the action is on fields in
> struct blk_trace, so having a lock inside that would make a lot more
> sense.
>
> It would also help to document what exactly we're actually protecting.

I think I documented in the patch that the lock has to protect changes
in the blktrace structure as well as the allocation and destruction of
it. Because of that, it can't be put inside the blktrace structure. The
original code use the bd_mutex of the block_device structure. I just
change the code to use another bd_fsfreeze_mutex in the same structure.

In an earlier patch version, I used a global blktrace mutex. This was
deemed to be not scalable enough and so I now use the bd_fsfreeze_mutex
instead.

Cheers,
Longman

[Cluster-devel] [PATCH] gfs2: Fix debugfs glocks dump

[Andreas Gruenbacher]

The switch to rhashtables (commit 88ffbf3e03) broke the debugfs glock
dump (/sys/kernel/debug/gfs2//glocks) for dumps bigger than a
single buffer: the right function for restarting an rhashtable iteration
from the beginning of the hash table is rhashtable_walk_enter;
rhashtable_walk_stop + rhashtable_walk_start will just resume from the
current position.

Signed-off-by: Andreas Gruenbacher 
---
 fs/gfs2/glock.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 98e845b7841b..11066d8647d2 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1945,13 +1945,9 @@ static void *gfs2_glock_seq_start(struct seq_file *seq, 
loff_t *pos)
 {
struct gfs2_glock_iter *gi = seq->private;
loff_t n = *pos;
-   int ret;
-
-   if (gi->last_pos <= *pos)
-   n = (*pos - gi->last_pos);
 
-   ret = rhashtable_walk_start(>hti);
-   if (ret)
+   rhashtable_walk_enter(_hash_table, >hti);
+   if (rhashtable_walk_start(>hti) != 0)
return NULL;
 
do {
@@ -1959,6 +1955,7 @@ static void *gfs2_glock_seq_start(struct seq_file *seq, 
loff_t *pos)
} while (gi->gl && n--);
 
gi->last_pos = *pos;
+
return gi->gl;
 }
 
@@ -1970,6 +1967,7 @@ static void *gfs2_glock_seq_next(struct seq_file *seq, 
void *iter_ptr,
(*pos)++;
gi->last_pos = *pos;
gfs2_glock_iter_next(gi);
+
return gi->gl;
 }
 
@@ -1980,6 +1978,7 @@ static void gfs2_glock_seq_stop(struct seq_file *seq, 
void *iter_ptr)
 
gi->gl = NULL;
rhashtable_walk_stop(>hti);
+   rhashtable_walk_exit(>hti);
 }
 
 static int gfs2_glock_seq_show(struct seq_file *seq, void *iter_ptr)
@@ -2042,12 +2041,10 @@ static int __gfs2_glocks_open(struct inode *inode, 
struct file *file,
struct gfs2_glock_iter *gi = seq->private;
 
gi->sdp = inode->i_private;
-   gi->last_pos = 0;
seq->buf = kmalloc(GFS2_SEQ_GOODSIZE, GFP_KERNEL | 
__GFP_NOWARN);
if (seq->buf)
seq->size = GFS2_SEQ_GOODSIZE;
gi->gl = NULL;
-   rhashtable_walk_enter(_hash_table, >hti);
}
return ret;
 }
@@ -2063,7 +2060,6 @@ static int gfs2_glocks_release(struct inode *inode, 
struct file *file)
struct gfs2_glock_iter *gi = seq->private;
 
gi->gl = NULL;
-   rhashtable_walk_exit(>hti);
return seq_release_private(inode, file);
 }
 
-- 
2.13.3