[Cluster-devel] [GFS2 PATCH 2/2] GFS2: Revert 35e478f Flush pending glock work when evicting an inode

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On 19/11/15 18:42, Bob Peterson wrote:
> Commit 35e478f was added because there were cases where
> the inode's glock still had pending delayed work.
> The theory is that the delayed work referenced the inode after it
> had been deleted from memory by gfs2_evict_inode during its call to
> filemap_fdatawrite. I'm guessing that's wrong. I'm guessing that
> the delayed work was referencing the glock after it had been
> freed due to the call to gfs2_glock_put() in gfs2_evict_inode().
> The previous patch made the last put be done from the delayed
> work queue, so that should make 35e478f obsolete. The other thing
> to consider is that vfs evict() calls invalidate_inode_buffers
> before calling the file system-specific gfs2_evict_inode(), and
> therefore we should have no dirty buffers for the inode itself.
> (We might have dirty buffers for the glock address space, though).
Not only are there likely to be dirty buffers still in the meta data 
address space, that is a requirement for good performance when unlinking 
lots of inodes.

> The reason why the call to flush_delayed_work can't be done here
> is that it can block, waiting like so:
>
> kernel: Call Trace:
> kernel: [<ffffffff815395f5>] schedule_timeout+0x215/0x2e0
> kernel: [<ffffffff81141f1a>] ? shrink_inactive_list+0x53a/0x830
> kernel: [<ffffffff81539273>] wait_for_common+0x123/0x180
> kernel: [<ffffffff810672b0>] ? default_wake_function+0x0/0x20
> kernel: [<ffffffff8153938d>] wait_for_completion+0x1d/0x20
> kernel: [<ffffffff8109b3e7>] flush_work+0x77/0xc0
> kernel: [<ffffffff8109ac50>] ? wq_barrier_func+0x0/0x20
> kernel: [<ffffffff8109b604>] flush_delayed_work+0x54/0x70
> kernel: [<ffffffffa05e3552>] gfs2_clear_inode+0x62/0xf0 [gfs2]
>
> The delayed work can depend on actions from DLM, and DLM can
> hang indefinitely on a fencing action.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>   fs/gfs2/super.c | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 46e5004..4a9b090 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -1613,7 +1613,6 @@ out:
>   	clear_inode(inode);
>   	gfs2_dir_hash_inval(ip);
>   	ip->i_gl->gl_object = NULL;
> -	flush_delayed_work(&ip->i_gl->gl_work);
>   	gfs2_glock_add_to_lru(ip->i_gl);
>   	if (queue_delayed_work(gfs2_glock_workqueue,
>   			       &ip->i_gl->gl_work, 0) == 0)

I'm not convinced that it is safe to do this, without finding some other 
way to flush the workqueue. All glock work items on the queue hold a ref 
count to the glock, so it should not be possible to have a case where 
the glock is referenced by work queue activity after it has been freed  
- unless I've misunderstood the explanation above,

Steve.