Hi, On Tue, 27 Nov 2018 at 20:26, Bob Peterson <rpete...@redhat.com> wrote: > Hi, > > This patch is based on an idea from Steve Whitehouse. The idea is > to dump the number of pages (both inode and its glock's address > space) in the glock dumps. To avoid adding any new locks, I > switch from calling i_size_read to using its constituant pieces, > namely, calling preempt_disable before gathering the values of > i_size and the new nrpages values, then I preempt_enable. > > Signed-off-by: Bob Peterson <rpete...@redhat.com> > --- > fs/gfs2/glops.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c > index f79ef9525e33..f60411707e88 100644 > --- a/fs/gfs2/glops.c > +++ b/fs/gfs2/glops.c > @@ -470,14 +470,28 @@ static int inode_go_lock(struct gfs2_holder *gh) > static void inode_go_dump(struct seq_file *seq, const struct gfs2_glock *gl) > { > const struct gfs2_inode *ip = gl->gl_object; > + const struct inode *inode;
The inode pointer can be directly initialized to &ip->i_inode even when inode is NULL. > + const struct address_space *mapping = (const struct address_space *) > + (gl + 1); /* Can't use gfs2_glock2aspace due to const */ This introduces code duplication, which seems worse than casting away const here. > + unsigned long nrpages1, nrpages2; > + loff_t i_size; > + > if (ip == NULL) > return; > - gfs2_print_dbg(seq, " I: n:%llu/%llu t:%u f:0x%02lx d:0x%08x > s:%llu\n", > + > + inode = &ip->i_inode; > + preempt_disable(); > + i_size = inode->i_size; > + nrpages1 = inode->i_data.nrpages; > + nrpages2 = mapping->nrpages; > + preempt_enable(); Please don't open-code i_size_read: it's a simple memory read on the architectures we really care about, and cheap enough on the others. The nrpages values are protected by xa_lock(&mapping->i_pages). If we really want to make sure we're always reporting correct values on 32-bit architectures, we'd probably have to read nrpages under lock. On 64-bit architectures, I believe we can directly read the nrpages. I agree that nrpages for the data address space can be interesting. Not sure if nrpages for the metadata address space is important enough to bother reporting it though. > + > + gfs2_print_dbg(seq, " I: n:%llu/%llu t:%u f:0x%02lx d:0x%08x s:%llu > p:%lu/%lu\n", > (unsigned long long)ip->i_no_formal_ino, > (unsigned long long)ip->i_no_addr, > IF2DT(ip->i_inode.i_mode), ip->i_flags, > (unsigned int)ip->i_diskflags, > - (unsigned long long)i_size_read(&ip->i_inode)); > + (unsigned long long)i_size, nrpages1, nrpages2); > } > > /** > Thanks, Andreas