[Cluster-devel] [GFS2 PATCH] GFS2: Take inode off order_write list when setting jdata flag

[Cluster-devel] [GFS2 PATCH] GFS2: Take inode off order_write list when setting jdata flag

[Bob Peterson]

Hi,

This patch fixes a deadlock caused when the jdata flag is set for
inodes that are already on the ordered write list. Since it is
on the ordered write list, log_flush calls gfs2_ordered_write which
calls filemap_fdatawrite. But since the inode had the jdata flag
set, that calls gfs2_jdata_writepages, which tries to start a new
transaction. A new transaction cannot be started because it tries
to acquire the log_flush rwsem which is already locked by the log
flush operation.

The bottom line is: We cannot switch an inode from ordered to jdata
until we eliminate any ordered data pages (via log flush) or any
log_flush operation afterward will create the circular dependency
above. So we need to flush the log before setting the diskflags to
switch the file mode, then we need to remove the inode from the
ordered writes list.

Before this patch, the log flush was done for jdata->ordered, but
that's wrong. If we're going from jdata to ordered, we don't need
to call gfs2_log_flush because the call to filemap_fdatawrite will
do it for us:

   filemap_fdatawrite() -> __filemap_fdatawrite_range()
      __filemap_fdatawrite_range() -> do_writepages()
         do_writepages() -> gfs2_jdata_writepages()
            gfs2_jdata_writepages() -> gfs2_log_flush()

This patch modifies function do_gfs2_set_flags so that if a file
has its jdata flag set, and it's already on the ordered write list,
the log will be flushed and it will be removed from the list
before setting the flag.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index c7a904a8fbb4..0e9b81acf191 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -256,7 +256,7 @@ static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask)
 			goto out;
 	}
 	if ((flags ^ new_flags) & GFS2_DIF_JDATA) {
-		if (flags & GFS2_DIF_JDATA)
+		if (new_flags & GFS2_DIF_JDATA)
 			gfs2_log_flush(sdp, ip->i_gl, NORMAL_FLUSH);
 		error = filemap_fdatawrite(inode->i_mapping);
 		if (error)
@@ -264,6 +264,8 @@ static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask)
 		error = filemap_fdatawait(inode->i_mapping);
 		if (error)
 			goto out;
+		if (new_flags & GFS2_DIF_JDATA)
+			gfs2_ordered_del_inode(ip);
 	}
 	error = gfs2_trans_begin(sdp, RES_DINODE, 0);
 	if (error)

[Cluster-devel] [GFS2 PATCH] GFS2: Take inode off order_write list when setting jdata flag

[Abhijith Das]

Hi,

Looks good. ACK.

Cheers!
--Abhi

----- Original Message -----
> From: "Bob Peterson" <rpeterso@redhat.com>
> To: "cluster-devel" <cluster-devel@redhat.com>
> Sent: Friday, October 6, 2017 8:39:08 AM
> Subject: [Cluster-devel] [GFS2 PATCH] GFS2: Take inode off order_write list when setting jdata flag
> 
> Hi,
> 
> This patch fixes a deadlock caused when the jdata flag is set for
> inodes that are already on the ordered write list. Since it is
> on the ordered write list, log_flush calls gfs2_ordered_write which
> calls filemap_fdatawrite. But since the inode had the jdata flag
> set, that calls gfs2_jdata_writepages, which tries to start a new
> transaction. A new transaction cannot be started because it tries
> to acquire the log_flush rwsem which is already locked by the log
> flush operation.
> 
> The bottom line is: We cannot switch an inode from ordered to jdata
> until we eliminate any ordered data pages (via log flush) or any
> log_flush operation afterward will create the circular dependency
> above. So we need to flush the log before setting the diskflags to
> switch the file mode, then we need to remove the inode from the
> ordered writes list.
> 
> Before this patch, the log flush was done for jdata->ordered, but
> that's wrong. If we're going from jdata to ordered, we don't need
> to call gfs2_log_flush because the call to filemap_fdatawrite will
> do it for us:
> 
>    filemap_fdatawrite() -> __filemap_fdatawrite_range()
>       __filemap_fdatawrite_range() -> do_writepages()
>          do_writepages() -> gfs2_jdata_writepages()
>             gfs2_jdata_writepages() -> gfs2_log_flush()
> 
> This patch modifies function do_gfs2_set_flags so that if a file
> has its jdata flag set, and it's already on the ordered write list,
> the log will be flushed and it will be removed from the list
> before setting the flag.
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index c7a904a8fbb4..0e9b81acf191 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -256,7 +256,7 @@ static int do_gfs2_set_flags(struct file *filp, u32
> reqflags, u32 mask)
>  			goto out;
>  	}
>  	if ((flags ^ new_flags) & GFS2_DIF_JDATA) {
> -		if (flags & GFS2_DIF_JDATA)
> +		if (new_flags & GFS2_DIF_JDATA)
>  			gfs2_log_flush(sdp, ip->i_gl, NORMAL_FLUSH);
>  		error = filemap_fdatawrite(inode->i_mapping);
>  		if (error)
> @@ -264,6 +264,8 @@ static int do_gfs2_set_flags(struct file *filp, u32
> reqflags, u32 mask)
>  		error = filemap_fdatawait(inode->i_mapping);
>  		if (error)
>  			goto out;
> +		if (new_flags & GFS2_DIF_JDATA)
> +			gfs2_ordered_del_inode(ip);
>  	}
>  	error = gfs2_trans_begin(sdp, RES_DINODE, 0);
>  	if (error)
> 
>