Hi,
On Tue, 27 Nov 2018 at 20:26, Bob Peterson 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
> ---
> 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
