Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAPt2mGNEYUk5u8V4abe=5MM5msZqmvzCVrtCP4Qw1n=gCHCnww@mail.gmail.com>
 <CALF+zO=e+d3sdLA4MZ_-SZh3epWBKF=hY=8FB+aB8+H4rxe4KA@mail.gmail.com>
 <Y++RA7YXtymaQJ05@casper.infradead.org> <CALF+zOnVJ5+Pb_mq1KcD_jdVsP8Dkg9KPGGiRS8KDJzK7+mT6Q@mail.gmail.com>
 <Y++kUMY93plkyP6f@casper.infradead.org>
In-Reply-To: <Y++kUMY93plkyP6f@casper.infradead.org>
From:   David Wysochanski <dwysocha@redhat.com>
Date:   Thu, 23 Mar 2023 13:56:38 -0400
Message-ID: <CALF+zO=J3d1DMvnaWKabpVhjXMa=wSmGo=swbm6iH9F+JUg5Xg@mail.gmail.com>
Subject: Re: /proc/PID/io/read_bytes accounting regression?
To:     David Howells <dhowells@redhat.com>
Cc:     Daire Byrne <daire@dneg.com>,
        linux-nfs <linux-nfs@vger.kernel.org>,
        Matthew Wilcox <willy@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Feb 17, 2023 at 10:59=E2=80=AFAM Matthew Wilcox <willy@infradead.or=
g> wrote:
>
> On Fri, Feb 17, 2023 at 10:47:01AM -0500, David Wysochanski wrote:
> > On Fri, Feb 17, 2023 at 9:36 AM Matthew Wilcox <willy@infradead.org> wr=
ote:
> > >
> > > On Fri, Feb 17, 2023 at 09:08:27AM -0500, David Wysochanski wrote:
> > > > On Fri, Feb 17, 2023 at 6:56 AM Daire Byrne <daire@dneg.com> wrote:
> > > > > On newer kernels, it looks like the task io accounting is not
> > > > > incrementing the read_bytes when reading from a NFS mount? This w=
as
> > > > > definitely working on v5.16 downwards, but has not been working s=
ince
> > > > > v5.18 up to v6.2 (I haven't tested v5.17 yet).
> > > >
> > > > In v5.16 we had this call to task_io_account_read(PAGE_SIZE); on li=
ne 109
> > > > of read_cache_pages();
> > > >
> > > > But there's no call to task_io_account_read() anymore in the new
> > > > readahead code paths that I could tell,
> > > > but maybe I'm missing something.
> > > >
> > > > Willy,
> > > > Does each caller of readahead_page() now need to call
> > > > task_io_account_read() or should we add that into
> > > > readahead_page() or maybe inside read_pages()?
> > >
> > > I think the best way is to mimic what the block layer does as closely=
 as
> > > possible.  Unless we can pull it out of the block layer & all
> > > filesystems and put it in the VFS (which we can't; the VFS doesn't
> > > know which blocks are recorded by the filesystem as holes and will no=
t
> > > result in I/O).
> > >
> > Caller, ok.  I see, that makes sense.
> > Looks like cifs has a call to task_io_account_read() after data has bee=
n
> > received.  Also netfs has a call as well at the bottom of
> > netfs_rreq_unlock_folios().
> > Both seems to be _after_ data has been received, but I'm not sure that'=
s
> > correct.
>
> It's probably correct, just different from the block layer.  I don't
> have any special insight here, just an inclination to be as similar
> as possible.
>
> > > The block layer does it as part of the BIO submission path (and also
> > > counts PGPGIN and PGPGOUT, which no network filesystems seem to do?)
> > > You're more familiar with the NFS code than I am, so you probably
> > > have a better idea than __nfs_pageio_add_request().
> > >
> > Hmm, yes does the block layer's accounting take into account actual
> > bytes read or just submitted?  Maybe I need to look at this closer
> > but at first glance it looks like these numbers may sometimes be
> > incremented when actual data is received and others are incremented
> > when the submission happens.
> >
> > As to the right location in NFS, the function you mention isn't a bad
> > idea, but maybe not the right location.  Looking in nfs_file_direct_rea=
d()
> > we have the accounting at IO submission time, appears to be the
> > same as the block layer.  Also since my NFS netfs conversion patches
> > are still outstanding, I'll have to somehow take the netfs call into ac=
count
> > if fscache is enabled.  So the right place is looking like somewhere
> > in nfs_read_folio() and nfs_readahead().
>
> Yes, we don't want to double-count either fscache or direct I/O.
> I'm Maybe Dave as opinions about where we should be accounting it --
> I'm not sure that netfs is the right place to do it.  Maybe it should
> be in each network filesystem instead of in netfs?
>

David - can you comment on whether you agree the call to
task_io_account_read() inside netfs_rreq_unlock_folios() is wrong?  I
think based on the discussion here, the definition in the kernel docs,
and the fact we have to record against the proper PID, the accounting
should be done in the caller of the netfs interfaces not inside netfs
unlock path.  It looks like afs may rely on netfs IO accounting but
can it be moved?