Re: [Cluster-devel] [PATCH] gfs2: Take inode glock exclusively when mounted without noatime
On Wed, Nov 25, 2020 at 9:49 AM Steven Whitehouse wrote:
> On 24/11/2020 16:42, Andreas Gruenbacher wrote:
> > Commit 20f82c38 ("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
Re: [Cluster-devel] [PATCH] gfs2: Take inode glock exclusively when mounted without noatime
Hi,
On 24/11/2020 16:42, Andreas Gruenbacher wrote:
Commit 20f82c38 ("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
Fixes: 20f82c38 ("gfs2: Rework read and page fault locking")
Cc: sta...@vger.kernel.org # v5.8+
Signed-off-by: Andreas Gruenbacher
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, );
+ state = IS_NOATIME(>i_inode) ? LM_ST_SHARED : LM_ST_EXCLUSIVE;
+ gfs2_holder_init(ip->i_gl, state, 0, );
ret = gfs2_glock_nq();
if (ret)
goto out_uninit;
[Cluster-devel] [PATCH] gfs2: Take inode glock exclusively when mounted without noatime
Commit 20f82c38 ("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
Fixes: 20f82c38 ("gfs2: Rework read and page fault locking")
Cc: sta...@vger.kernel.org # v5.8+
Signed-off-by: Andreas Gruenbacher
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, );
+ state = IS_NOATIME(>i_inode) ? LM_ST_SHARED : LM_ST_EXCLUSIVE;
+ gfs2_holder_init(ip->i_gl, state, 0, );
ret = gfs2_glock_nq();
if (ret)
goto out_uninit;
