All of lore.kernel.org
 help / color / mirror / Atom feed
From: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
To: Li RongQing <roy.qing.li@gmail.com>,
	Trond Myklebust <trond.myklebust@primarydata.com>,
	Anna Schumaker <anna.schumaker@netapp.com>
Cc: linux-nfs@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: nfs: wrong function to increment NFSIOS_READPAGES/WRITEPAGES stat counters?
Date: Tue, 24 Mar 2015 20:53:16 +0800	[thread overview]
Message-ID: <55115E3C.2030104@m4x.org> (raw)

Hello,

Since commit 5a254d08b086 ("nfs: replace nfs_add_stats with
nfs_inc_stats when add one") [1], nfs_readpage and nfs_do_writepage  use
nfs_inc_stats to increment NFSIOS_READPAGES and NFSIOS_WRITEPAGES
instead of nfs_add_stats.

However nfs_inc_stats is not similar as nfs_add_stats with parameter 1
because these functions work on distinct stats:
nfs_inc_stats increments stats from "enum nfs_stat_eventcounters" (in
server->io_stats->events) and nfs_add_stats those from "enum
nfs_stat_bytecounters" (in server->io_stats->bytes), according to their
implementations in fs/nfs/iostat.h [2].

If I understand the code correctly, "nfs_inc_stats(inode,
NFSIOS_READPAGES);" is in fact executed as "nfs_inc_stats(inode,
NFSIOS_VFSACCESS);" and "nfs_inc_stats(inode, NFSIOS_WRITEPAGES);" as
"nfs_inc_stats(inode, NFSIOS_VFSUPDATEPAGE);".

As this looks like a bug to me, I am reporting it in hope it could be
fixed, for example by reverting 5a254d08b086.  Of course, I may be
wrong, in which case an explanation about what I missed in my analysis
would be appreciated.


By the way, I found this while trying to compile LLVMLinux on current
master branch of Linux.  clang reported the following warnings:

fs/nfs/read.c:287:23: warning: implicit conversion from enumeration type
'enum nfs_stat_bytecounters' to different enumeration type 'enum
nfs_stat_eventcounters' [-Wenum-conversion]
            nfs_inc_stats(inode, NFSIOS_READPAGES);
            ~~~~~~~~~~~~~        ^~~~~~~~~~~~~~~~

fs/nfs/write.c:583:23: warning: implicit conversion from enumeration
type 'enum nfs_stat_bytecounters' to different enumeration type 'enum
nfs_stat_eventcounters' [-Wenum-conversion]
            nfs_inc_stats(inode, NFSIOS_WRITEPAGES);
            ~~~~~~~~~~~~~        ^~~~~~~~~~~~~~~~~

Thanks,

Nicolas

[1]
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5a254d08b086d80cbead2ebcee6d2a4b3a15587a
[2]
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/nfs/iostat.h?id=90a5a895cc8b284ac522757a01de15e36710c2b9

             reply	other threads:[~2015-03-24 12:53 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-24 12:53 Nicolas Iooss [this message]
2015-04-16 10:48 ` [PATCH] Revert "nfs: replace nfs_add_stats with nfs_inc_stats when add one" Nicolas Iooss

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55115E3C.2030104@m4x.org \
    --to=nicolas.iooss_linux@m4x.org \
    --cc=anna.schumaker@netapp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=roy.qing.li@gmail.com \
    --cc=trond.myklebust@primarydata.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.