Re: 回复：regression caused by patch 6180bb446ab624b9ab8bf201ed251ca87f07b413?? ("dax: fix detection of dax support for non-persistent memory block?? devices")

[Dan Williams <dan.j.williams@intel.com>]

On Tue, Sep 15, 2020 at 12:58 PM Mike Snitzer <snitzer@redhat.com> wrote:
>
> On Tue, Sep 15 2020 at  3:49pm -0400,
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > On Tue, Sep 15, 2020 at 1:01 AM Jan Kara <jack@suse.cz> wrote:
> > >
> > > Hi!
> > >
> > > On Tue 15-09-20 11:03:29, colyli@suse.de wrote:
> > > > Could you please to take a look? I am offline in the next two weeks.
> > >
> > > I just had a look into this. IMHO the justification in 6180bb446a "dax: fix
> > > detection of dax support for non-persistent memory block devices" is just
> > > bogus and people got confused by the previous condition
> > >
> > > if (!dax_dev && !bdev_dax_supported(bdev, blocksize))
> > >
> > > which was bogus as well. bdev_dax_supported() always returns false for bdev
> > > that doesn't have dax_dev (naturally so). So in the original condition
> > > there was no point in calling bdev_dax_supported() if we know dax_dev is
> > > NULL.
> > >
> > > Then this was changed to:
> > >
> > > if (!dax_dev || !bdev_dax_supported(bdev, blocksize))
> > >
> > > which looks more sensible at the first sight. But only at the first sight -
> > > if you look at wider context, __generic_fsdax_supported() is the bulk of
> > > code that decides whether a device supports DAX so calling
> > > bdev_dax_supported() from it indeed doesn't look as such a great idea. So
> > > IMO the condition should be just:
> > >
> > > if (!dax_dev)
> > >
> > > I'll send a fix for this.
> >
> > If you beat me to it, great, but you might be sleeping now. I agree
> > the original condition was bogus and looks to be a result of previous
> > non-thorough refactoring on my part. I think we can move that !dax_dev
> > into dax_supported(). I'll take a look.
>
> You trimmed the relevant portion of Jan's reply

Vishal already owned up to the process escape.

> but: can you also
> weigh-in one whether DM is using the wrong function to test for DAX?

That might be the original source of the bug because I would expect DM
to be using dax_supported() which should route to all the component
devices. I'll have a talk with whomever put that call to
generic_fsdax_supported() in DM:

git blame drivers/md/dm-table.c
...
7bf7eac8d6480 (Dan Williams         2019-05-16 13:26:29 -0700  865)
 return generic_fsdax_supported(dev->dax_dev, dev->bdev, blocksize,
...

oh...
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org