From: Jan Kara <jack@suse.cz>
To: Ira Weiny <ira.weiny@intel.com>
Cc: Jan Kara <jack@suse.cz>, Michal Hocko <mhocko@suse.com>,
Theodore Ts'o <tytso@mit.edu>,
linux-nvdimm@lists.01.org, linux-rdma@vger.kernel.org,
John Hubbard <jhubbard@nvidia.com>,
Dave Chinner <david@fromorbit.com>,
linux-kernel@vger.kernel.org,
Matthew Wilcox <willy@infradead.org>,
linux-xfs@vger.kernel.org, Jason Gunthorpe <jgg@ziepe.ca>,
linux-mm@kvack.org, linux-fsdevel@vger.kernel.org,
Andrew Morton <akpm@linux-foundation.org>,
linux-ext4@vger.kernel.org
Subject: Re: [RFC PATCH v2 00/19] RDMA/FS DAX truncate proposal V1,000,002 ; -)
Date: Mon, 19 Aug 2019 08:36:58 +0200 [thread overview]
Message-ID: <20190819063658.GB20455@quack2.suse.cz> (raw)
In-Reply-To: <20190816232006.GA11384@iweiny-DESK2.sc.intel.com>
On Fri 16-08-19 16:20:07, Ira Weiny wrote:
> On Fri, Aug 16, 2019 at 12:05:28PM -0700, 'Ira Weiny' wrote:
> > On Thu, Aug 15, 2019 at 03:05:58PM +0200, Jan Kara wrote:
> > > On Wed 14-08-19 11:08:49, Ira Weiny wrote:
> > > > On Wed, Aug 14, 2019 at 12:17:14PM +0200, Jan Kara wrote:
> > > > > Hello!
> > > > >
> > > > > On Fri 09-08-19 15:58:14, ira.weiny@intel.com wrote:
> > > > > > Pre-requisites
> > > > > > ==============
> > > > > > Based on mmotm tree.
> > > > > >
> > > > > > Based on the feedback from LSFmm, the LWN article, the RFC series since
> > > > > > then, and a ton of scenarios I've worked in my mind and/or tested...[1]
> > > > > >
> > > > > > Solution summary
> > > > > > ================
> > > > > >
> > > > > > The real issue is that there is no use case for a user to have RDMA pinn'ed
> > > > > > memory which is then truncated. So really any solution we present which:
> > > > > >
> > > > > > A) Prevents file system corruption or data leaks
> > > > > > ...and...
> > > > > > B) Informs the user that they did something wrong
> > > > > >
> > > > > > Should be an acceptable solution.
> > > > > >
> > > > > > Because this is slightly new behavior. And because this is going to be
> > > > > > specific to DAX (because of the lack of a page cache) we have made the user
> > > > > > "opt in" to this behavior.
> > > > > >
> > > > > > The following patches implement the following solution.
> > > > > >
> > > > > > 0) Registrations to Device DAX char devs are not affected
> > > > > >
> > > > > > 1) The user has to opt in to allowing page pins on a file with an exclusive
> > > > > > layout lease. Both exclusive and layout lease flags are user visible now.
> > > > > >
> > > > > > 2) page pins will fail if the lease is not active when the file back page is
> > > > > > encountered.
> > > > > >
> > > > > > 3) Any truncate or hole punch operation on a pinned DAX page will fail.
> > > > >
> > > > > So I didn't fully grok the patch set yet but by "pinned DAX page" do you
> > > > > mean a page which has corresponding file_pin covering it? Or do you mean a
> > > > > page which has pincount increased? If the first then I'd rephrase this to
> > > > > be less ambiguous, if the second then I think it is wrong.
> > > >
> > > > I mean the second. but by "fail" I mean hang. Right now the "normal" page
> > > > pincount processing will hang the truncate. Given the discussion with John H
> > > > we can make this a bit better if we use something like FOLL_PIN and the page
> > > > count bias to indicate this type of pin. Then I could fail the truncate
> > > > outright. but that is not done yet.
> > > >
> > > > so... I used the word "fail" to be a bit more vague as the final implementation
> > > > may return ETXTBUSY or hang as noted.
> > >
> > > Ah, OK. Hanging is fine in principle but with longterm pins, your work
> > > makes sure they actually fail with ETXTBUSY, doesn't it? The thing is that
> > > e.g. DIO will use page pins as well for its buffers and we must wait there
> > > until the pin is released. So please just clarify your 'fail' here a bit
> > > :).
> >
> > It will fail with ETXTBSY. I've fixed a bug... See below.
> >
> > >
> > > > > > 4) The user has the option of holding the lease or releasing it. If they
> > > > > > release it no other pin calls will work on the file.
> > > > >
> > > > > Last time we spoke the plan was that the lease is kept while the pages are
> > > > > pinned (and an attempt to release the lease would block until the pages are
> > > > > unpinned). That also makes it clear that the *lease* is what is making
> > > > > truncate and hole punch fail with ETXTBUSY and the file_pin structure is
> > > > > just an implementation detail how the existence is efficiently tracked (and
> > > > > what keeps the backing file for the pages open so that the lease does not
> > > > > get auto-destroyed). Why did you change this?
> > > >
> > > > closing the file _and_ unmaping it will cause the lease to be released
> > > > regardless of if we allow this or not.
> > > >
> > > > As we discussed preventing the close seemed intractable.
> > >
> > > Yes, preventing the application from closing the file is difficult. But
> > > from a quick look at your patches it seemed to me that you actually hold a
> > > backing file reference from the file_pin structure thus even though the
> > > application closes its file descriptor, the struct file (and thus the
> > > lease) lives further until the file_pin gets released. And that should last
> > > as long as the pages are pinned. Am I missing something?
> > >
> > > > I thought about failing the munmap but that seemed wrong as well. But more
> > > > importantly AFAIK RDMA can pass its memory pins to other processes via FD
> > > > passing... This means that one could pin this memory, pass it to another
> > > > process and exit. The file lease on the pin'ed file is lost.
> > >
> > > Not if file_pin grabs struct file reference as I mentioned above...
> > >
> > > > The file lease is just a key to get the memory pin. Once unlocked the procfs
> > > > tracking keeps track of where that pin goes and which processes need to be
> > > > killed to get rid of it.
> > >
> > > I think having file lease being just a key to get the pin is conceptually
> > > wrong. The lease is what expresses: "I'm accessing these blocks directly,
> > > don't touch them without coordinating with me." So it would be only natural
> > > if we maintained the lease while we are accessing blocks instead of
> > > transferring this protection responsibility to another structure - namely
> > > file_pin - and letting the lease go.
> >
> > We do transfer that protection to the file_pin but we don't have to "let the
> > lease" go. We just keep the lease with the file_pin as you said. See below...
> >
> > > But maybe I miss some technical reason
> > > why maintaining file lease is difficult. If that's the case, I'd like to hear
> > > what...
> >
> > Ok, I've thought a bit about what you said and indeed it should work that way.
> > The reason I had to think a bit is that I was not sure why I thought we needed
> > to hang... Turns out there were a couple of reasons... 1 not so good and 1 ok
> > but still not good enough to allow this...
> >
> > 1) I had a bug in the XFS code which should have failed rather than hanging...
> > So this was not a good reason... And I was able to find/fix it... Thanks!
> >
> > 2) Second reason is that I thought I did not have a good way to tell if the
> > lease was actually in use. What I mean is that letting the lease go should
> > be ok IFF we don't have any pins... I was thinking that without John's code
> > we don't have a way to know if there are any pins... But that is wrong...
> > All we have to do is check
> >
> > !list_empty(file->file_pins)
>
> Oops... I got my "struct files" mixed up... The RDMA struct file has the
> file_pins hanging off it... This will not work.
>
> I'll have to try something else to prevent this. However, I don't want to walk
> all the pages of the inode.
>
> Also I'm concerned about just failing if they happen to be pinned. They need
> to be LONGTERM pinned... Otherwise we might have a transient failure of an
> unlock based on some internal kernel transient pin... :-/
My solution for this was that file_pin would contain counter of pinned
pages which vaddr_pin_pages() would increment and vaddr_unpin_pages() would
decrement. Checking whether there's any outstanding page pinned attached to
the file_pin is then trivial...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2019-08-19 6:37 UTC|newest]
Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-09 22:58 [RFC PATCH v2 00/19] RDMA/FS DAX truncate proposal V1,000,002 ;-) ira.weiny
2019-08-09 22:58 ` [RFC PATCH v2 01/19] fs/locks: Export F_LAYOUT lease to user space ira.weiny
2019-08-09 23:52 ` Dave Chinner
2019-08-12 17:36 ` Ira Weiny
2019-08-14 8:05 ` Dave Chinner
2019-08-14 11:21 ` Jeff Layton
2019-08-14 11:38 ` Dave Chinner
2019-08-09 22:58 ` [RFC PATCH v2 02/19] fs/locks: Add Exclusive flag to user Layout lease ira.weiny
2019-08-14 14:15 ` Jeff Layton
2019-08-14 21:56 ` Dave Chinner
2019-08-26 10:41 ` Jeff Layton
2019-08-29 23:34 ` Ira Weiny
2019-09-04 12:52 ` Jeff Layton
2019-09-04 23:12 ` John Hubbard
2019-08-09 22:58 ` [RFC PATCH v2 03/19] mm/gup: Pass flags down to __gup_device_huge* calls ira.weiny
2019-08-09 22:58 ` [RFC PATCH v2 04/19] mm/gup: Ensure F_LAYOUT lease is held prior to GUP'ing pages ira.weiny
2019-08-09 22:58 ` [RFC PATCH v2 05/19] fs/ext4: Teach ext4 to break layout leases ira.weiny
2019-08-09 22:58 ` [RFC PATCH v2 06/19] fs/ext4: Teach dax_layout_busy_page() to operate on a sub-range ira.weiny
2019-08-23 15:18 ` Vivek Goyal
2019-08-29 18:52 ` Ira Weiny
2019-08-09 22:58 ` [RFC PATCH v2 07/19] fs/xfs: Teach xfs to use new dax_layout_busy_page() ira.weiny
2019-08-09 23:30 ` Dave Chinner
2019-08-12 18:05 ` Ira Weiny
2019-08-14 8:04 ` Dave Chinner
2019-08-09 22:58 ` [RFC PATCH v2 08/19] fs/xfs: Fail truncate if page lease can't be broken ira.weiny
2019-08-09 23:22 ` Dave Chinner
2019-08-12 18:08 ` Ira Weiny
2019-08-09 22:58 ` [RFC PATCH v2 09/19] mm/gup: Introduce vaddr_pin structure ira.weiny
2019-08-10 0:06 ` John Hubbard
2019-08-09 22:58 ` [RFC PATCH v2 10/19] mm/gup: Pass a NULL vaddr_pin through GUP fast ira.weiny
2019-08-10 0:06 ` John Hubbard
2019-08-09 22:58 ` [RFC PATCH v2 11/19] mm/gup: Pass follow_page_context further down the call stack ira.weiny
2019-08-10 0:18 ` John Hubbard
2019-08-12 19:01 ` Ira Weiny
2019-08-09 22:58 ` [RFC PATCH v2 12/19] mm/gup: Prep put_user_pages() to take an vaddr_pin struct ira.weiny
2019-08-10 0:30 ` John Hubbard
2019-08-12 20:46 ` Ira Weiny
2019-08-09 22:58 ` [RFC PATCH v2 13/19] {mm,file}: Add file_pins objects ira.weiny
2019-08-09 22:58 ` [RFC PATCH v2 14/19] fs/locks: Associate file pins while performing GUP ira.weiny
2019-08-09 22:58 ` [RFC PATCH v2 15/19] mm/gup: Introduce vaddr_pin_pages() ira.weiny
2019-08-10 0:09 ` John Hubbard
2019-08-12 21:00 ` Ira Weiny
2019-08-12 21:20 ` John Hubbard
2019-08-11 23:07 ` John Hubbard
2019-08-12 21:01 ` Ira Weiny
2019-08-12 12:28 ` Jason Gunthorpe
2019-08-12 21:48 ` Ira Weiny
2019-08-13 11:47 ` Jason Gunthorpe
2019-08-13 17:46 ` Ira Weiny
2019-08-13 17:56 ` John Hubbard
2019-08-09 22:58 ` [RFC PATCH v2 16/19] RDMA/uverbs: Add back pointer to system file object ira.weiny
2019-08-12 13:00 ` Jason Gunthorpe
2019-08-12 17:28 ` Ira Weiny
2019-08-12 17:56 ` Jason Gunthorpe
2019-08-12 21:15 ` Ira Weiny
2019-08-13 11:48 ` Jason Gunthorpe
2019-08-13 17:41 ` Ira Weiny
2019-08-13 18:00 ` Jason Gunthorpe
2019-08-13 20:38 ` Ira Weiny
2019-08-14 12:23 ` Jason Gunthorpe
2019-08-14 17:50 ` Ira Weiny
2019-08-14 18:15 ` Jason Gunthorpe
2019-09-04 22:25 ` Ira Weiny
2019-09-11 8:19 ` Jason Gunthorpe
2019-08-09 22:58 ` [RFC PATCH v2 17/19] RDMA/umem: Convert to vaddr_[pin|unpin]* operations ira.weiny
2019-08-09 22:58 ` [RFC PATCH v2 18/19] {mm,procfs}: Add display file_pins proc ira.weiny
2019-08-09 22:58 ` [RFC PATCH v2 19/19] mm/gup: Remove FOLL_LONGTERM DAX exclusion ira.weiny
2019-08-14 10:17 ` [RFC PATCH v2 00/19] RDMA/FS DAX truncate proposal V1,000,002 ;-) Jan Kara
2019-08-14 18:08 ` Ira Weiny
2019-08-15 13:05 ` Jan Kara
2019-08-16 19:05 ` Ira Weiny
2019-08-16 23:20 ` [RFC PATCH v2 00/19] RDMA/FS DAX truncate proposal V1,000,002 ; -) Ira Weiny
2019-08-19 6:36 ` Jan Kara [this message]
2019-08-17 2:26 ` [RFC PATCH v2 00/19] RDMA/FS DAX truncate proposal V1,000,002 ;-) Dave Chinner
2019-08-19 6:34 ` Jan Kara
2019-08-19 9:24 ` Dave Chinner
2019-08-19 12:38 ` Jason Gunthorpe
2019-08-19 21:53 ` Ira Weiny
2019-08-20 1:12 ` Dave Chinner
2019-08-20 11:55 ` Jason Gunthorpe
2019-08-21 18:02 ` Ira Weiny
2019-08-21 18:13 ` Jason Gunthorpe
2019-08-21 18:22 ` John Hubbard
2019-08-21 18:57 ` Ira Weiny
2019-08-21 19:06 ` Ira Weiny
2019-08-21 19:48 ` Jason Gunthorpe
2019-08-21 20:44 ` Ira Weiny
2019-08-21 23:49 ` Jason Gunthorpe
2019-08-23 3:23 ` Dave Chinner
2019-08-23 12:04 ` Jason Gunthorpe
2019-08-24 0:11 ` Dave Chinner
2019-08-24 5:08 ` Ira Weiny
2019-08-26 5:55 ` Dave Chinner
2019-08-29 2:02 ` Ira Weiny
2019-08-29 3:27 ` John Hubbard
2019-08-29 16:16 ` Ira Weiny
2019-09-02 22:26 ` Dave Chinner
2019-09-04 16:54 ` Ira Weiny
2019-08-25 19:39 ` Jason Gunthorpe
2019-08-24 4:49 ` Ira Weiny
2019-08-25 19:40 ` Jason Gunthorpe
2019-08-23 0:59 ` Dave Chinner
2019-08-23 17:15 ` Ira Weiny
2019-08-24 0:18 ` Dave Chinner
2019-08-20 0:05 ` John Hubbard
2019-08-20 1:20 ` Dave Chinner
2019-08-20 3:09 ` John Hubbard
2019-08-20 3:36 ` Dave Chinner
2019-08-21 18:43 ` John Hubbard
2019-08-21 19:09 ` Ira Weiny
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=20190819063658.GB20455@quack2.suse.cz \
--to=jack@suse.cz \
--cc=akpm@linux-foundation.org \
--cc=david@fromorbit.com \
--cc=ira.weiny@intel.com \
--cc=jgg@ziepe.ca \
--cc=jhubbard@nvidia.com \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-nvdimm@lists.01.org \
--cc=linux-rdma@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=mhocko@suse.com \
--cc=tytso@mit.edu \
--cc=willy@infradead.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).