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>
In-Reply-To: <Y++RA7YXtymaQJ05@casper.infradead.org>
From:   David Wysochanski <dwysocha@redhat.com>
Date:   Fri, 17 Feb 2023 10:47:01 -0500
Message-ID: <CALF+zOnVJ5+Pb_mq1KcD_jdVsP8Dkg9KPGGiRS8KDJzK7+mT6Q@mail.gmail.com>
Subject: Re: /proc/PID/io/read_bytes accounting regression?
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Daire Byrne <daire@dneg.com>, linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Feb 17, 2023 at 9:36 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> 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 was
> > > definitely working on v5.16 downwards, but has not been working since
> > > 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 line 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 not
> 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 been
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.

> 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_read()
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 account
if fscache is enabled.  So the right place is looking like somewhere
in nfs_read_folio() and nfs_readahead().