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;
Date:   Thu, 1 Sep 2022 11:06:28 +0200
From:   Jan Kara <jack@suse.cz>
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Jan Kara <jack@suse.cz>, John Hubbard <jhubbard@nvidia.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Jens Axboe <axboe@kernel.dk>,
        Miklos Szeredi <miklos@szeredi.hu>,
        Christoph Hellwig <hch@infradead.org>,
        "Darrick J . Wong" <djwong@kernel.org>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>,
        Logan Gunthorpe <logang@deltatee.com>,
        linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-xfs@vger.kernel.org, linux-nfs@vger.kernel.org,
        linux-mm@kvack.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 5/6] NFS: direct-io: convert to FOLL_PIN pages
Message-ID: <20220901090628.h4debwejkirrhqtj@quack3>
References: <20220827083607.2345453-6-jhubbard@nvidia.com>
 <YwqfWoAE2Awp4YvT@ZenIV>
 <353f18ac-0792-2cb7-6675-868d0bd41d3d@nvidia.com>
 <Ywq5ILRNxsbWvFQe@ZenIV>
 <Ywq5VrSrY341UVpL@ZenIV>
 <217b4a17-1355-06c5-291e-7980c0d3cea6@nvidia.com>
 <20220829160808.rwkkiuelipr3huxk@quack3>
 <a53b2d14-687a-16c9-2f63-4f94876f8b3c@nvidia.com>
 <20220831094349.boln4jjajkdtykx3@quack3>
 <Yw/+/U9GFaNnARdk@ZenIV>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yw/+/U9GFaNnARdk@ZenIV>
Precedence: bulk

On Thu 01-09-22 01:38:21, Al Viro wrote:
> On Wed, Aug 31, 2022 at 11:43:49AM +0200, Jan Kara wrote:
> 
> > So after looking into that a bit more, I think a clean approach would be to
> > provide iov_iter_pin_pages2() and iov_iter_pages_alloc2(), under the hood
> > in __iov_iter_get_pages_alloc() make sure we use pin_user_page() instead of
> > get_page() in all the cases (using this in pipe_get_pages() and
> > iter_xarray_get_pages() is easy) and then make all bio handling use the
> > pinning variants for iters. I think at least iov_iter_is_pipe() case needs
> > to be handled as well because as I wrote above, pipe pages can enter direct
> > IO code e.g. for splice(2).
> > 
> > Also I think that all iov_iter_get_pages2() (or the _alloc2 variant) users
> > actually do want the "pin page" semantics in the end (they are accessing
> > page contents) so eventually we should convert them all to
> > iov_iter_pin_pages2() and remove iov_iter_get_pages2() altogether. But this
> > will take some more conversion work with networking etc. so I'd start with
> > converting bios only.
> 
> Not sure, TBH...
> 
> FWIW, quite a few of the callers of iov_iter_get_pages2() do *NOT* need to
> grab any references for BVEC/XARRAY/PIPE cases.  What's more, it would be
> bloody useful to have a variant that doesn't grab references for
> !iter->user_backed case - that could be usable for KVEC as well, simplifying
> several callers.
> 
> Requirements:
> 	* recepients of those struct page * should have a way to make
> dropping the page refs conditional (obviously); bio machinery can be told
> to do so.
> 	* callers should *NOT* do something like
> 	"set an ITER_BVEC iter, with page references grabbed and stashed in
> bio_vec array, call async read_iter() and drop the references in array - the
> refs we grab in dio will serve"
> Note that for sync IO that pattern is fine whether we grab/drop anything
> inside read_iter(); for async we could take depopulating the bio_vec
> array to the IO completion or downstream of that.
> 	* the code dealing with the references returned by iov_iter_..._pages
> should *NOT* play silly buggers with refcounts - something like "I'll grab
> a reference, start DMA and report success; page will stay around until I
> get around to dropping the ref and callers don't need to wait for that" deep
> in the bowels of infinibad stack (or something equally tasteful) is seriously
> asking for trouble.

I agree we could get away without grabbing references in some cases. But it
is a performance vs robustness tradeoff I'd say. E.g. with XARRAY case I
can see people feeding pages from struct address_space and it is unclear
what else than page reference would protect them from being freed by
reclaim. Furthermore if e.g. writeback can happen for such struct
address_space (or other filesystem operations requiring stable data), we
really need a full page pin and not just page reference to signal that the
page may be under DMA and may be changing under your hands. So I'm not
against having some special cases that avoid grabbing page reference / page
pin but I think it is justified only in performance sensitive cases and
when we can make sure filesystem backed page (because of above mentioned
data stability issues) or anon page (because of cow handling) cannot
possibly enter this path.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR