[Cluster-devel] [PATCH] gfs2: Take inode glock exclusively when mounted without noatime

[Cluster-devel] [PATCH] gfs2: Take inode glock exclusively when mounted without noatime

[Andreas Gruenbacher]

Commit 20f829999c38 ("gfs2: Rework read and page fault locking") has lifted the
glock lock taking from the low-level ->readpage and ->readahead address space
operations to the higher-level ->read_iter file and ->fault vm operations.  The
glocks are still taken in LM_ST_SHARED mode only.  On filesystems mounted
without the noatime option, ->read_iter needs to update the atime as well
though, so we currently run into a failed locking mode assertion in
gfs2_dirty_inode.  Fix that by taking the glock in LM_ST_EXCLUSIVE mode on
filesystems mounted without the noatime mount option.

Faulting in pages doesn't update the atime, so in the ->fault vm operation,
taking the glock in LM_ST_SHARED mode is enough.

Reported-by: Alexander Aring <aahringo@redhat.com>
Fixes: 20f829999c38 ("gfs2: Rework read and page fault locking")
Cc: stable at vger.kernel.org # v5.8+
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index b39b339feddc..162a81873dcd 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -849,6 +849,7 @@ static ssize_t gfs2_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	struct gfs2_inode *ip;
 	struct gfs2_holder gh;
 	size_t written = 0;
+	unsigned int state;
 	ssize_t ret;
 
 	if (iocb->ki_flags & IOCB_DIRECT) {
@@ -871,7 +872,8 @@ static ssize_t gfs2_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 			return ret;
 	}
 	ip = GFS2_I(iocb->ki_filp->f_mapping->host);
-	gfs2_holder_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
+	state = IS_NOATIME(&ip->i_inode) ? LM_ST_SHARED : LM_ST_EXCLUSIVE;
+	gfs2_holder_init(ip->i_gl, state, 0, &gh);
 	ret = gfs2_glock_nq(&gh);
 	if (ret)
 		goto out_uninit;

[Cluster-devel] [PATCH] gfs2: Take inode glock exclusively when mounted without noatime

[Steven Whitehouse]

Hi,

On 24/11/2020 16:42, Andreas Gruenbacher wrote:
> Commit 20f829999c38 ("gfs2: Rework read and page fault locking") has lifted the
> glock lock taking from the low-level ->readpage and ->readahead address space
> operations to the higher-level ->read_iter file and ->fault vm operations.  The
> glocks are still taken in LM_ST_SHARED mode only.  On filesystems mounted
> without the noatime option, ->read_iter needs to update the atime as well
> though, so we currently run into a failed locking mode assertion in
> gfs2_dirty_inode.  Fix that by taking the glock in LM_ST_EXCLUSIVE mode on
> filesystems mounted without the noatime mount option.
>
> Faulting in pages doesn't update the atime, so in the ->fault vm operation,
> taking the glock in LM_ST_SHARED mode is enough.

I don't think this makes any sense to do. It is going to reduce the 
scalibility quite a lot I suspect. Even if you have multiple nodes 
reading a file, the atime updates would not be synchronous with the 
reads, so why insist on an exclusive lock here?

Steve.


>
> Reported-by: Alexander Aring <aahringo@redhat.com>
> Fixes: 20f829999c38 ("gfs2: Rework read and page fault locking")
> Cc: stable at vger.kernel.org # v5.8+
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
>
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index b39b339feddc..162a81873dcd 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -849,6 +849,7 @@ static ssize_t gfs2_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
>   	struct gfs2_inode *ip;
>   	struct gfs2_holder gh;
>   	size_t written = 0;
> +	unsigned int state;
>   	ssize_t ret;
>   
>   	if (iocb->ki_flags & IOCB_DIRECT) {
> @@ -871,7 +872,8 @@ static ssize_t gfs2_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
>   			return ret;
>   	}
>   	ip = GFS2_I(iocb->ki_filp->f_mapping->host);
> -	gfs2_holder_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
> +	state = IS_NOATIME(&ip->i_inode) ? LM_ST_SHARED : LM_ST_EXCLUSIVE;
> +	gfs2_holder_init(ip->i_gl, state, 0, &gh);
>   	ret = gfs2_glock_nq(&gh);
>   	if (ret)
>   		goto out_uninit;
>

[Cluster-devel] [PATCH] gfs2: Take inode glock exclusively when mounted without noatime

[Andreas Gruenbacher]

On Wed, Nov 25, 2020 at 9:49 AM Steven Whitehouse <swhiteho@redhat.com> wrote:
> On 24/11/2020 16:42, Andreas Gruenbacher wrote:
> > Commit 20f829999c38 ("gfs2: Rework read and page fault locking") has lifted the
> > glock lock taking from the low-level ->readpage and ->readahead address space
> > operations to the higher-level ->read_iter file and ->fault vm operations.  The
> > glocks are still taken in LM_ST_SHARED mode only.  On filesystems mounted
> > without the noatime option, ->read_iter needs to update the atime as well
> > though, so we currently run into a failed locking mode assertion in
> > gfs2_dirty_inode.  Fix that by taking the glock in LM_ST_EXCLUSIVE mode on
> > filesystems mounted without the noatime mount option.
> >
> > Faulting in pages doesn't update the atime, so in the ->fault vm operation,
> > taking the glock in LM_ST_SHARED mode is enough.
>
> I don't think this makes any sense to do. It is going to reduce the
> scalibility quite a lot I suspect. Even if you have multiple nodes
> reading a file, the atime updates would not be synchronous with the
> reads, so why insist on an exclusive lock here?

Indeed, it's a bit slow, even though I'm not sure where you're going
with that "not synchronous" argument. I've replaced this with a patch
that upgrades a shared lock to an exclusive lock only when the atime
actually needs to be updated; see "gfs2: Upgrade shared glocks for
atime updates".

(Note that the revised patch isn't easy to test without
instrumentation because usually, the atime update will happen when we
try to read from the page cache, in which case gfs2_update_time will
be called without holding the glock.)

Thanks,
Andreas