Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Dan Williams <dan.j.williams@intel.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: linux-nvdimm <linux-nvdimm@lists.01.org>, Jan Kara <jack@suse.cz>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] dax: Fix Xarray conversion of dax_unlock_mapping_entry()
Date: Fri, 30 Nov 2018 12:05:13 -0800
Message-ID: <CAPcyv4iqA84WbE8pE=Yi15Q0T8JUXnPZss27HqhQTP08F9sTrQ@mail.gmail.com> (raw)
In-Reply-To: <20181130195021.GN10377@bombadil.infradead.org>

On Fri, Nov 30, 2018 at 11:50 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Nov 30, 2018 at 09:01:07AM -0800, Dan Williams wrote:
> > On Fri, Nov 30, 2018 at 8:33 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > On Fri, Nov 30, 2018 at 8:24 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Fri, Nov 30, 2018 at 07:54:49AM -0800, Dan Williams wrote:
> > > > > Looks good to me, although can we make that cookie an actual type? I
> > > > > think it's mostly ok to pass around (void *) for 'entry' inside of
> > > > > fs/dax.c, but once an entry leaves that file I'd like it to have an
> > > > > explicit type to catch people that might accidentally pass a (struct
> > > > > page *) to the unlock routine.
> > > >
> > > > That's a really good idea.  Something like this?
> > > >
> > > > typedef struct {
> > > >         void *v;
> > > > } dax_entry_t;
> > >
> > > Yes, please.
>
> Oh.  The caller needs to interpret it to see if the entry was successfully
> locked, so it needs to be an integer type (or we need a comparison
> function ... bleh).
>
> > > > I could see us making good use of that within dax.c.
> >
> > I'm now thinking that this is a nice improvement for 4.21. For 4.20-rc
> > lets do the localized fix.
>
> I think both patches are equally risky.  I admit this patch crosses a
> file boundary, but the other patch changes dax_make_entry() which is
> used by several functions which aren't part of this path, whereas this
> patch only changes functions used in the path which is known to be buggy.

I'm almost buying this argument... but I'd feel better knowing that
all dax_make_entry() usages are safe against this bug pattern. I
didn't audit the other occurrences of dax_make_entry() for this bug,
did you build some confidence here?

>
> This patch has the advantage of getting us closer to where we want to be
> sooner.

One comment below...

>
>
> From 1135b8d08f23ab4f5b28261535a817f3de9297c9 Mon Sep 17 00:00:00 2001
> From: Matthew Wilcox <willy@infradead.org>
> Date: Fri, 30 Nov 2018 11:05:06 -0500
> Subject: [PATCH] dax: Change lock/unlock API
>
> Return the unlock cookie from dax_lock_mapping_entry() and
> pass it to dax_unlock_mapping_entry().  This fixes a bug where
> dax_unlock_mapping_entry() was assuming that the page was PMD-aligned
> if the entry was a PMD entry.
>
> Debugged-by: Dan Williams <dan.j.williams@intel.com>
> Fixes: 9f32d221301c ("dax: Convert dax_lock_mapping_entry to XArray")
> Signed-off-by: Matthew Wilcox <willy@infradead.org>
> ---
>  fs/dax.c            | 21 ++++++++-------------
>  include/linux/dax.h | 15 +++++++++------
>  mm/memory-failure.c |  6 ++++--
>  3 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/fs/dax.c b/fs/dax.c
> index 9bcce89ea18e..d2c04e802978 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -351,20 +351,20 @@ static struct page *dax_busy_page(void *entry)
>   * @page: The page whose entry we want to lock
>   *
>   * Context: Process context.
> - * Return: %true if the entry was locked or does not need to be locked.
> + * Return: A cookie to pass to dax_unlock_mapping_entry() or 0 if the
> + * entry could not be locked.
>   */
> -bool dax_lock_mapping_entry(struct page *page)
> +dax_entry_t dax_lock_mapping_entry(struct page *page)
>  {
>         XA_STATE(xas, NULL, 0);
>         void *entry;
> -       bool locked;
>
>         /* Ensure page->mapping isn't freed while we look at it */
>         rcu_read_lock();
>         for (;;) {
>                 struct address_space *mapping = READ_ONCE(page->mapping);
>
> -               locked = false;
> +               entry = NULL;
>                 if (!dax_mapping(mapping))
>                         break;
>
> @@ -375,7 +375,7 @@ bool dax_lock_mapping_entry(struct page *page)
>                  * otherwise we would not have a valid pfn_to_page()
>                  * translation.
>                  */
> -               locked = true;
> +               entry = (void *)~0UL;
>                 if (S_ISCHR(mapping->host->i_mode))
>                         break;
>
> @@ -400,23 +400,18 @@ bool dax_lock_mapping_entry(struct page *page)
>                 break;
>         }
>         rcu_read_unlock();
> -       return locked;
> +       return (dax_entry_t)entry;
>  }
>
> -void dax_unlock_mapping_entry(struct page *page)
> +void dax_unlock_mapping_entry(struct page *page, dax_entry_t entry)

Let's not require the page to be passed back, it can be derived:

    page = pfn_to_page(dax_to_pfn((void*) entry));

A bit more symmetric that way and canonical with other locking schemes
that return a cookie.

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-30  0:13 Dan Williams
2018-11-30 15:49 ` Matthew Wilcox
2018-11-30 15:54   ` Dan Williams
2018-11-30 16:24     ` Matthew Wilcox
2018-11-30 16:33       ` Dan Williams
2018-11-30 17:01         ` Dan Williams
2018-11-30 19:50           ` Matthew Wilcox
2018-11-30 20:05             ` Dan Williams [this message]
2018-12-04  3:33               ` Dan Williams
2018-12-05  1:34                 ` Matthew Wilcox
2018-12-05  6:11                   ` Dan Williams
2018-12-05  9:22                     ` Jan Kara

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='CAPcyv4iqA84WbE8pE=Yi15Q0T8JUXnPZss27HqhQTP08F9sTrQ@mail.gmail.com' \
    --to=dan.j.williams@intel.com \
    --cc=jack@suse.cz \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvdimm@lists.01.org \
    --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

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git