Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320964 > unrolled thread

[PATCH 2/2] dax: fix bdev NULL pointer dereferences

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-01-28 20:40 +0100
    Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-01-28 21:30 +0100
    Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Christoph Hellwig <hch@infradead.org> - 2016-01-28 22:40 +0100
      Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-01-29 19:30 +0100
        Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-01-30 00:40 +0100
          Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-01-30 01:20 +0100
          Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dave Chinner <david@fromorbit.com> - 2016-01-31 23:50 +0100
        Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Matthew Wilcox <willy@linux.intel.com> - 2016-01-30 06:30 +0100
          Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-01-30 07:10 +0100
            Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jared Hulbert <jaredeh@gmail.com> - 2016-01-30 08:10 +0100
            Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Matthew Wilcox <willy@linux.intel.com> - 2016-01-31 03:40 +0100
              Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <zwisler@gmail.com> - 2016-01-31 07:20 +0100
                Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Matthew Wilcox <willy@linux.intel.com> - 2016-01-31 12:00 +0100
                  Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-01-31 17:40 +0100
                    Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Matthew Wilcox <willy@linux.intel.com> - 2016-01-31 19:10 +0100
                      Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-01-31 19:20 +0100
                        Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Matthew Wilcox <willy@linux.intel.com> - 2016-01-31 19:30 +0100
                          Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-01-31 20:00 +0100
                      Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-01-31 21:00 +0100
          Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jan Kara <jack@suse.cz> - 2016-02-01 16:00 +0100
            Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Matthew Wilcox <willy@linux.intel.com> - 2016-02-01 21:50 +0100
            Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dave Chinner <david@fromorbit.com> - 2016-02-01 22:50 +0100
              Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jared Hulbert <jaredeh@gmail.com> - 2016-02-02 07:10 +0100
                Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-02-02 07:50 +0100
                  Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jared Hulbert <jaredeh@gmail.com> - 2016-02-02 09:10 +0100
                    Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-02-02 18:00 +0100
                      Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jared Hulbert <jaredeh@gmail.com> - 2016-02-02 22:50 +0100
                        Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Matthew Wilcox <willy@linux.intel.com> - 2016-02-03 01:40 +0100
                          Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jared Hulbert <jaredeh@gmail.com> - 2016-02-03 02:30 +0100
              Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jan Kara <jack@suse.cz> - 2016-02-02 12:20 +0100
                Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-02-02 17:40 +0100
                  Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jan Kara <jack@suse.cz> - 2016-02-02 17:50 +0100
                    Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-02-02 18:20 +0100
                      Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-02 18:40 +0100
                        Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-02-02 18:50 +0100
                          Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-02 19:30 +0100
                        Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-02-02 18:50 +0100
                          Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Matthew Wilcox <willy@linux.intel.com> - 2016-02-02 19:50 +0100
                            Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dan Williams <dan.j.williams@intel.com> - 2016-02-02 20:10 +0100
                              Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Matthew Wilcox <willy@linux.intel.com> - 2016-02-02 21:20 +0100
                            Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jan Kara <jack@suse.cz> - 2016-02-03 12:10 +0100
                        Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jan Kara <jack@suse.cz> - 2016-02-03 11:50 +0100
                          Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-03 21:20 +0100
                            Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jan Kara <jack@suse.cz> - 2016-02-04 10:20 +0100
                              Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-05 00:40 +0100
                              Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dave Chinner <david@fromorbit.com> - 2016-02-07 00:20 +0100
                                Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-07 06:30 +0100
                          Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-04 21:00 +0100
                            Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jan Kara <jack@suse.cz> - 2016-02-04 21:40 +0100
                              Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-04 23:20 +0100
                              Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-05 23:30 +0100
                                Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dave Chinner <david@fromorbit.com> - 2016-02-07 00:50 +0100
                                  Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-07 07:50 +0100
                                    Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jan Kara <jack@suse.cz> - 2016-02-08 14:50 +0100
                                Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Christoph Hellwig <hch@infradead.org> - 2016-02-07 09:40 +0100
                                  Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-08 17:00 +0100
                Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-02 19:50 +0100
                  Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-02 20:00 +0100
      Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-02 01:10 +0100
        Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Dave Chinner <david@fromorbit.com> - 2016-02-02 08:20 +0100
        Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Jan Kara <jack@suse.cz> - 2016-02-02 11:40 +0100

Page 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

#1323496

On Mon, Feb 01, 2016 at 03:51:47PM +0100, Jan Kara wrote:
> On Sat 30-01-16 00:28:33, Matthew Wilcox wrote:
> > I think we're doing it wrong by storing the sector in the radix tree; we'd
> > really need to store both the sector and the bdev which is too much data.
> > 
> > If we store the PFN of the underlying page instead, we don't have this
> > problem.  Instead, we have a different problem; of the device going
> > away under us.  I'm trying to find the code which tears down PTEs when
> > the device goes away, and I'm not seeing it.  What do we do about user
> > mappings of the device?
> 
> So I don't have a strong opinion whether storing PFN or sector is better.
> Maybe PFN is somewhat more generic but OTOH turning DAX off for special
> cases like inodes on XFS RT devices would be IMHO fine.

I'm not sure that's such a great idea.  RT devices might be the best
way to get aligned pages on storage.

> I'm somewhat concerned that there are several things in flight (page fault
> rework, invalidation on device removal, issues with DAX access to block
> devices Ross found) and this is IMHO the smallest trouble we have and changing
> this seems relatively invasive.

This isn't the minimal change to convert us from storing sectors to
storing PFNs.  This is a wholescale rework based around using the page
cache radix tree as the primary data structure instead of buffer heads.

> So could we settle the fault code and
> similar stuff first and look into this somewhat later? Because frankly I
> have some trouble following how all the pieces are going to fit together
> and I'm afraid we'll introduce some non-trivial bugs when several
> fundamental things are in flux in parallel.

We can, of course, do a much smaller patch that will use the radix tree
much less centrally.  And that might be the right way to go for 4.5.
With the extra couple of months we'll have, I hope that this redesign
can be the basis for the DAX code in 4.6.

[toc] | [prev] | [next] | [standalone]

#1323539

On Mon, Feb 01, 2016 at 03:51:47PM +0100, Jan Kara wrote:
> On Sat 30-01-16 00:28:33, Matthew Wilcox wrote:
> > On Fri, Jan 29, 2016 at 11:28:15AM -0700, Ross Zwisler wrote:
> > > I guess I need to go off and understand if we can have DAX mappings on such a
> > > device.  If we can, we may have a problem - we can get the block_device from
> > > get_block() in I/O path and the various fault paths, but we don't have access
> > > to get_block() when flushing via dax_writeback_mapping_range().  We avoid
> > > needing it the normal case by storing the sector results from get_block() in
> > > the radix tree.
> > 
> > I think we're doing it wrong by storing the sector in the radix tree; we'd
> > really need to store both the sector and the bdev which is too much data.
> > 
> > If we store the PFN of the underlying page instead, we don't have this
> > problem.  Instead, we have a different problem; of the device going
> > away under us.  I'm trying to find the code which tears down PTEs when
> > the device goes away, and I'm not seeing it.  What do we do about user
> > mappings of the device?
> 
> So I don't have a strong opinion whether storing PFN or sector is better.
> Maybe PFN is somewhat more generic but OTOH turning DAX off for special
> cases like inodes on XFS RT devices would be IMHO fine.

We need to support alternate devices.

There is a strong case for using the XFS RT device with DAX,
especially for applications that know they are going to always use
large/huge/giant pages to access their data files. The XFS RT device
can guarantee allocation is always aligned to large/huge/giant page
constraints right up to ENOSPC and throughout the production life of
the filesystem. We have no other filesystem capable of providing
such guarantees, which means the XFS RT device is uniquely suited to
certain aplications with DAX...

> I'm somewhat concerned that there are several things in flight (page fault
> rework, invalidation on device removal, issues with DAX access to block
> devices Ross found) and this is IMHO the smallest trouble we have and changing
> this seems relatively invasive. So could we settle the fault code and
> similar stuff first and look into this somewhat later? Because frankly I
> have some trouble following how all the pieces are going to fit together
> and I'm afraid we'll introduce some non-trivial bugs when several
> fundamental things are in flux in parallel.

Yup, there's way to many balls in the air at the moment.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1323810

On Mon, Feb 1, 2016 at 1:47 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Mon, Feb 01, 2016 at 03:51:47PM +0100, Jan Kara wrote:
>> On Sat 30-01-16 00:28:33, Matthew Wilcox wrote:
>> > On Fri, Jan 29, 2016 at 11:28:15AM -0700, Ross Zwisler wrote:
>> > > I guess I need to go off and understand if we can have DAX mappings on such a
>> > > device.  If we can, we may have a problem - we can get the block_device from
>> > > get_block() in I/O path and the various fault paths, but we don't have access
>> > > to get_block() when flushing via dax_writeback_mapping_range().  We avoid
>> > > needing it the normal case by storing the sector results from get_block() in
>> > > the radix tree.
>> >
>> > I think we're doing it wrong by storing the sector in the radix tree; we'd
>> > really need to store both the sector and the bdev which is too much data.
>> >
>> > If we store the PFN of the underlying page instead, we don't have this
>> > problem.  Instead, we have a different problem; of the device going
>> > away under us.  I'm trying to find the code which tears down PTEs when
>> > the device goes away, and I'm not seeing it.  What do we do about user
>> > mappings of the device?
>>
>> So I don't have a strong opinion whether storing PFN or sector is better.
>> Maybe PFN is somewhat more generic but OTOH turning DAX off for special
>> cases like inodes on XFS RT devices would be IMHO fine.
>
> We need to support alternate devices.

Embedded devices trying to use NOR Flash to free up RAM was
historically one of the more prevalent real world uses of the old
filemap_xip.c code although the users never made it to mainline.  So I
spent some time last week trying to figure out how to make a subset of
DAX not depend on CONFIG_BLOCK.  It was a very frustrating and
unfruitful experience.  I discarded my main conclusion as impractical,
but now that I see the difficultly DAX faces in dealing with
"alternate devices" especially some of the crazy stuff btrfs can do, I
wonder if it's not so crazy after all.

Lets stop calling bdev_direct_access() directly from DAX.  Let the
filesystems do it.

Sure we could enable generic_dax_direct_access() helper for the
filesystems that only support single devices to make it easy.  But XFS
and btrfs for example, have to do the work of figuring out what bdev
is required and then calling bdev_direct_access().

My reasoning is that the filesystem knows how to map inodes and
offsets to devices and sectors, no matter how complex that is.  It
would even enable a filesystem to intelligently use a mix of
direct_access and regular block devices down the road.  Of course it
would also make the block-less solution doable.

Good idea?  Stupid idea?

[toc] | [prev] | [next] | [standalone]

#1323834

On Mon, Feb 1, 2016 at 10:06 PM, Jared Hulbert <jaredeh@gmail.com> wrote:
> On Mon, Feb 1, 2016 at 1:47 PM, Dave Chinner <david@fromorbit.com> wrote:
>> On Mon, Feb 01, 2016 at 03:51:47PM +0100, Jan Kara wrote:
>>> On Sat 30-01-16 00:28:33, Matthew Wilcox wrote:
>>> > On Fri, Jan 29, 2016 at 11:28:15AM -0700, Ross Zwisler wrote:
>>> > > I guess I need to go off and understand if we can have DAX mappings on such a
>>> > > device.  If we can, we may have a problem - we can get the block_device from
>>> > > get_block() in I/O path and the various fault paths, but we don't have access
>>> > > to get_block() when flushing via dax_writeback_mapping_range().  We avoid
>>> > > needing it the normal case by storing the sector results from get_block() in
>>> > > the radix tree.
>>> >
>>> > I think we're doing it wrong by storing the sector in the radix tree; we'd
>>> > really need to store both the sector and the bdev which is too much data.
>>> >
>>> > If we store the PFN of the underlying page instead, we don't have this
>>> > problem.  Instead, we have a different problem; of the device going
>>> > away under us.  I'm trying to find the code which tears down PTEs when
>>> > the device goes away, and I'm not seeing it.  What do we do about user
>>> > mappings of the device?
>>>
>>> So I don't have a strong opinion whether storing PFN or sector is better.
>>> Maybe PFN is somewhat more generic but OTOH turning DAX off for special
>>> cases like inodes on XFS RT devices would be IMHO fine.
>>
>> We need to support alternate devices.
>
> Embedded devices trying to use NOR Flash to free up RAM was
> historically one of the more prevalent real world uses of the old
> filemap_xip.c code although the users never made it to mainline.  So I
> spent some time last week trying to figure out how to make a subset of
> DAX not depend on CONFIG_BLOCK.  It was a very frustrating and
> unfruitful experience.  I discarded my main conclusion as impractical,
> but now that I see the difficultly DAX faces in dealing with
> "alternate devices" especially some of the crazy stuff btrfs can do, I
> wonder if it's not so crazy after all.
>
> Lets stop calling bdev_direct_access() directly from DAX.  Let the
> filesystems do it.
>
> Sure we could enable generic_dax_direct_access() helper for the
> filesystems that only support single devices to make it easy.  But XFS
> and btrfs for example, have to do the work of figuring out what bdev
> is required and then calling bdev_direct_access().
>
> My reasoning is that the filesystem knows how to map inodes and
> offsets to devices and sectors, no matter how complex that is.  It
> would even enable a filesystem to intelligently use a mix of
> direct_access and regular block devices down the road.  Of course it
> would also make the block-less solution doable.
>
> Good idea?  Stupid idea?

The CONFIG_BLOCK=y case isn't going anywhere, so if anything it seems
the CONFIG_BLOCK=n is an incremental feature in its own right.  What
driver and what filesystem are looking to enable this XIP support in?

[toc] | [prev] | [next] | [standalone]

#1323870

On Mon, Feb 1, 2016 at 10:46 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Mon, Feb 1, 2016 at 10:06 PM, Jared Hulbert <jaredeh@gmail.com> wrote:
>> On Mon, Feb 1, 2016 at 1:47 PM, Dave Chinner <david@fromorbit.com> wrote:
>>> On Mon, Feb 01, 2016 at 03:51:47PM +0100, Jan Kara wrote:
>>>> On Sat 30-01-16 00:28:33, Matthew Wilcox wrote:
>>>> > On Fri, Jan 29, 2016 at 11:28:15AM -0700, Ross Zwisler wrote:
>>>> > > I guess I need to go off and understand if we can have DAX mappings on such a
>>>> > > device.  If we can, we may have a problem - we can get the block_device from
>>>> > > get_block() in I/O path and the various fault paths, but we don't have access
>>>> > > to get_block() when flushing via dax_writeback_mapping_range().  We avoid
>>>> > > needing it the normal case by storing the sector results from get_block() in
>>>> > > the radix tree.
>>>> >
>>>> > I think we're doing it wrong by storing the sector in the radix tree; we'd
>>>> > really need to store both the sector and the bdev which is too much data.
>>>> >
>>>> > If we store the PFN of the underlying page instead, we don't have this
>>>> > problem.  Instead, we have a different problem; of the device going
>>>> > away under us.  I'm trying to find the code which tears down PTEs when
>>>> > the device goes away, and I'm not seeing it.  What do we do about user
>>>> > mappings of the device?
>>>>
>>>> So I don't have a strong opinion whether storing PFN or sector is better.
>>>> Maybe PFN is somewhat more generic but OTOH turning DAX off for special
>>>> cases like inodes on XFS RT devices would be IMHO fine.
>>>
>>> We need to support alternate devices.
>>
>> Embedded devices trying to use NOR Flash to free up RAM was
>> historically one of the more prevalent real world uses of the old
>> filemap_xip.c code although the users never made it to mainline.  So I
>> spent some time last week trying to figure out how to make a subset of
>> DAX not depend on CONFIG_BLOCK.  It was a very frustrating and
>> unfruitful experience.  I discarded my main conclusion as impractical,
>> but now that I see the difficultly DAX faces in dealing with
>> "alternate devices" especially some of the crazy stuff btrfs can do, I
>> wonder if it's not so crazy after all.
>>
>> Lets stop calling bdev_direct_access() directly from DAX.  Let the
>> filesystems do it.
>>
>> Sure we could enable generic_dax_direct_access() helper for the
>> filesystems that only support single devices to make it easy.  But XFS
>> and btrfs for example, have to do the work of figuring out what bdev
>> is required and then calling bdev_direct_access().
>>
>> My reasoning is that the filesystem knows how to map inodes and
>> offsets to devices and sectors, no matter how complex that is.  It
>> would even enable a filesystem to intelligently use a mix of
>> direct_access and regular block devices down the road.  Of course it
>> would also make the block-less solution doable.
>>
>> Good idea?  Stupid idea?
>
> The CONFIG_BLOCK=y case isn't going anywhere, so if anything it seems
> the CONFIG_BLOCK=n is an incremental feature in its own right.  What
> driver and what filesystem are looking to enable this XIP support in?

Well... as CONFIG_BLOCK was not required with filemap_xip.c for a
decade.  This CONFIG_BLOCK dependency is a result of an incremental
feature from a certain point of view ;)

The obvious 'driver' is physical RAM without a particular driver.
Remember please I'm talking about embedded.  RAM measured in MiB and
funky one off hardware etc.  In the embedded world there are lots of
ways that persistent memory has been supported in device specific ways
without the new fancypants NFIT and Intel instructions, so frankly
they don't fit in the PMEM stuff.  Maybe they could be supported in
PMEM but not without effort to bring embedded players to the table.

The other drivers are the MTD drivers, probably as read-only for now.
But the paradigm there isn't so different from what PMEM looks like
with asymmetric read/write capabilities.

The filesystem I'm concerned with is AXFS
(https://www.kernel.org/doc/ols/2008/ols2008v1-pages-211-218.pdf).
Which I've been planning on trying to merge again due to a recent
resurgence of interest.  The device model for AXFS is... weird.  It
can use one or two devices at a time of any mix of NOR MTD, NAND MTD,
block, and unmanaged physical memory.  It's a terribly useful model
for embedded.  Anyway AXFS is readonly so hacking in a read only
dax_fault_nodev() and dax_file_read() would work fine, looks easy
enough.  But... it would be cool if similar small embedded focused RW
filesystems were enabled.

I don't expect you to taint DAX with design requirements for this
stuff that it wasn't built for, nobody ends up happy in that case.
However, if enabling the filesystem to manage the bdev_direct_access()
interactions solves some of the "alternate device" problems you are
discussing here, then there is a chance we can accommodate both.
Sometimes that works.

So... Forget CONFIG_BLOCK=n entirely I didn't want that to be the
focus anyway.  Does it help to support the weirder XFS and btrfs
device models to enable the filesystem to handle the
bdev_direct_access() stuff?

[toc] | [prev] | [next] | [standalone]

#1324278

On Tue, Feb 2, 2016 at 12:05 AM, Jared Hulbert <jaredeh@gmail.com> wrote:
[..]
> Well... as CONFIG_BLOCK was not required with filemap_xip.c for a
> decade.  This CONFIG_BLOCK dependency is a result of an incremental
> feature from a certain point of view ;)
>
> The obvious 'driver' is physical RAM without a particular driver.
> Remember please I'm talking about embedded.  RAM measured in MiB and
> funky one off hardware etc.  In the embedded world there are lots of
> ways that persistent memory has been supported in device specific ways
> without the new fancypants NFIT and Intel instructions,so frankly
> they don't fit in the PMEM stuff.  Maybe they could be supported in
> PMEM but not without effort to bring embedded players to the table.

Not sure what you're trying to say here.  An ACPI NFIT only feeds the
generic libnvdimm device model.  You don't need NFIT to get pmem.

> The other drivers are the MTD drivers, probably as read-only for now.
> But the paradigm there isn't so different from what PMEM looks like
> with asymmetric read/write capabilities.
>
> The filesystem I'm concerned with is AXFS
> (https://www.kernel.org/doc/ols/2008/ols2008v1-pages-211-218.pdf).
> Which I've been planning on trying to merge again due to a recent
> resurgence of interest.  The device model for AXFS is... weird.  It
> can use one or two devices at a time of any mix of NOR MTD, NAND MTD,
> block, and unmanaged physical memory.  It's a terribly useful model
> for embedded.  Anyway AXFS is readonly so hacking in a read only
> dax_fault_nodev() and dax_file_read() would work fine, looks easy
> enough.  But... it would be cool if similar small embedded focused RW
> filesystems were enabled.

Are those also out of tree?

> I don't expect you to taint DAX with design requirements for this
> stuff that it wasn't built for, nobody ends up happy in that case.
> However, if enabling the filesystem to manage the bdev_direct_access()
> interactions solves some of the "alternate device" problems you are
> discussing here, then there is a chance we can accommodate both.
> Sometimes that works.
>
> So... Forget CONFIG_BLOCK=n entirely I didn't want that to be the
> focus anyway.  Does it help to support the weirder XFS and btrfs
> device models to enable the filesystem to handle the
> bdev_direct_access() stuff?

It's not clear that it does.  We just clarified with xfs and ext4 that
we can really on get_blocks().  That solves the immediate concern with
multi-device filesystems.

[toc] | [prev] | [next] | [standalone]

#1324672

On Tue, Feb 2, 2016 at 8:51 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Tue, Feb 2, 2016 at 12:05 AM, Jared Hulbert <jaredeh@gmail.com> wrote:
> [..]
>> Well... as CONFIG_BLOCK was not required with filemap_xip.c for a
>> decade.  This CONFIG_BLOCK dependency is a result of an incremental
>> feature from a certain point of view ;)
>>
>> The obvious 'driver' is physical RAM without a particular driver.
>> Remember please I'm talking about embedded.  RAM measured in MiB and
>> funky one off hardware etc.  In the embedded world there are lots of
>> ways that persistent memory has been supported in device specific ways
>> without the new fancypants NFIT and Intel instructions,so frankly
>> they don't fit in the PMEM stuff.  Maybe they could be supported in
>> PMEM but not without effort to bring embedded players to the table.
>
> Not sure what you're trying to say here.  An ACPI NFIT only feeds the
> generic libnvdimm device model.  You don't need NFIT to get pmem.

Right... I'm just not seeing how the libnvdimm device model fits, is
relevant, or useful to a persistent SRAM in embedded.  Therefore I
don't see some of the user will have a driver.

>> The other drivers are the MTD drivers, probably as read-only for now.
>> But the paradigm there isn't so different from what PMEM looks like
>> with asymmetric read/write capabilities.
>>
>> The filesystem I'm concerned with is AXFS
>> (https://www.kernel.org/doc/ols/2008/ols2008v1-pages-211-218.pdf).
>> Which I've been planning on trying to merge again due to a recent
>> resurgence of interest.  The device model for AXFS is... weird.  It
>> can use one or two devices at a time of any mix of NOR MTD, NAND MTD,
>> block, and unmanaged physical memory.  It's a terribly useful model
>> for embedded.  Anyway AXFS is readonly so hacking in a read only
>> dax_fault_nodev() and dax_file_read() would work fine, looks easy
>> enough.  But... it would be cool if similar small embedded focused RW
>> filesystems were enabled.
>
> Are those also out of tree?

Of course.  Merging embedded filesystems is little merging regular
filesystems except 98% of you reviewers don't want it merged.

>> I don't expect you to taint DAX with design requirements for this
>> stuff that it wasn't built for, nobody ends up happy in that case.
>> However, if enabling the filesystem to manage the bdev_direct_access()
>> interactions solves some of the "alternate device" problems you are
>> discussing here, then there is a chance we can accommodate both.
>> Sometimes that works.
>>
>> So... Forget CONFIG_BLOCK=n entirely I didn't want that to be the
>> focus anyway.  Does it help to support the weirder XFS and btrfs
>> device models to enable the filesystem to handle the
>> bdev_direct_access() stuff?
>
> It's not clear that it does.  We just clarified with xfs and ext4 that
> we can really on get_blocks().  That solves the immediate concern with
> multi-device filesystems.

IMO you're making DAX more complex by overly coupling to the bdev and
I think it could bite you later.  I submit this rework of the radix
tree and confusion about where to get the real bdev as evidence.  I'm
guessing that it won't be the last time.  It's unnecessary to couple
it like this, and in fact is not how the vfs has been layered in the
past.

The trouble with vfs work has been that it straddles the line between
mm and block, unfortunately that line is dark chasm with ill defined
boundaries.  DAX is even more exciting because it's trying to duct
tape the filesystem even closer to the mm system, one could argue it's
actually in some respects enabling the filesystem to bypass the mm
code.  On top of that DAX is designed to enable block based
filesystems to use RAM like devices.

Bolting the block device interface on to NVDIMM is a brilliant hack
and the right design choice, but it's still a hack.  The upside is it
enables the reuse of all this glorious legacy filesystem code which
does a pretty amazing job of handling what the pmem device
applications need considering they were designed to manage data on
platters of slow spinning rust.  How would DAX look like developed
with a filesystem purpose built for pmem?

To look at the the downside consider dax_fault().  Its called on a
fault to a user memory map, uses the filesystems get_block() to lookup
a sector so you can ask a block device to convert it to an address on
a DIMM.  Come on, that's awkward.  Everything around dax_fault() is
dripping with memory semantic interfaces, the dax_fault() call are
fundamentally about memory, the pmem calls are memory, the hardware is
memory, and yet it directly calls bdev_direct_access().  It's out of
place.

The legacy vfs/mm code didn't have this layering problem either.  Even
filemap_fault() that dax_fault() is modeled after doesn't call any
bdev methods directly, when it needs something it asks the filesystem
with a ->readpage().  The precedence is that you ask the filesystem
for what you need.  Look at the get_bdev() thing you've concluded you
need.  It _almost_ makes my point.  I just happen to be of the opinion
that you don't actually want or need the bdev, you want the pfn/kaddr
so you can flush or map or memcpy().

[toc] | [prev] | [next] | [standalone]

#1324765

On Tue, Feb 02, 2016 at 01:46:06PM -0800, Jared Hulbert wrote:
> On Tue, Feb 2, 2016 at 8:51 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> >> The filesystem I'm concerned with is AXFS
> >> (https://www.kernel.org/doc/ols/2008/ols2008v1-pages-211-218.pdf).
> >> Which I've been planning on trying to merge again due to a recent
> >> resurgence of interest.  The device model for AXFS is... weird.  It
> >> can use one or two devices at a time of any mix of NOR MTD, NAND MTD,
> >> block, and unmanaged physical memory.  It's a terribly useful model
> >> for embedded.  Anyway AXFS is readonly so hacking in a read only
> >> dax_fault_nodev() and dax_file_read() would work fine, looks easy
> >> enough.  But... it would be cool if similar small embedded focused RW
> >> filesystems were enabled.
> >
> > Are those also out of tree?
> 
> Of course.  Merging embedded filesystems is little merging regular
> filesystems except 98% of you reviewers don't want it merged.

You should at least be able to get it into staging these days.  I mean,
look at some of the junk that's in staging ... and I don't think AXFS was
nearly as bad.

> IMO you're making DAX more complex by overly coupling to the bdev and
> I think it could bite you later.  I submit this rework of the radix
> tree and confusion about where to get the real bdev as evidence.  I'm
> guessing that it won't be the last time.  It's unnecessary to couple
> it like this, and in fact is not how the vfs has been layered in the
> past.

Huh?  The rework to use the radix tree for PFNs was done with one eye
firmly on your usage case.  Just because I had to thread the get_block
interface through it for the moment doesn't mean that I didn't have
the "how do we get rid of get_block entirely" question on my mind.

Using get_block seemed like the right idea three years ago.  I didn't
know just how fundamentally ext4 and XFS disagree on how it should be
used.

> To look at the the downside consider dax_fault().  Its called on a
> fault to a user memory map, uses the filesystems get_block() to lookup
> a sector so you can ask a block device to convert it to an address on
> a DIMM.  Come on, that's awkward.  Everything around dax_fault() is
> dripping with memory semantic interfaces, the dax_fault() call are
> fundamentally about memory, the pmem calls are memory, the hardware is
> memory, and yet it directly calls bdev_direct_access().  It's out of
> place.

What was out of place was the old 'get_xip_mem' in address_space
operations.  Returning a kernel virtual address and a PFN from a
filesystem operation?  That looks awful.  All the other operations deal
in struct pages, file offsets and occasionally sectors.  Of course, we
don't have a struct page, so a pfn makes sense, but the kernel virtual
address being returned was a gargantuan layering problem.

> The legacy vfs/mm code didn't have this layering problem either.  Even
> filemap_fault() that dax_fault() is modeled after doesn't call any
> bdev methods directly, when it needs something it asks the filesystem
> with a ->readpage().  The precedence is that you ask the filesystem
> for what you need.  Look at the get_bdev() thing you've concluded you
> need.  It _almost_ makes my point.  I just happen to be of the opinion
> that you don't actually want or need the bdev, you want the pfn/kaddr
> so you can flush or map or memcpy().

You want the pfn.  The device driver doesn't have enough information to
give you a (coherent with userspace) kaddr.  That's what (some future
arch-specific implementation of) dax_map_pfn() is for.  That's why it
takes 'index' as a parameter, so you can calculate where it'll be mapped
in userspace, and determine an appropriate kernel virtual address to
use for it.

[toc] | [prev] | [next] | [standalone]

#1324804

On Tue, Feb 2, 2016 at 4:34 PM, Matthew Wilcox <willy@linux.intel.com> wrote:
> On Tue, Feb 02, 2016 at 01:46:06PM -0800, Jared Hulbert wrote:
>> On Tue, Feb 2, 2016 at 8:51 AM, Dan Williams <dan.j.williams@intel.com> wrote:
>> >> The filesystem I'm concerned with is AXFS
>> >> (https://www.kernel.org/doc/ols/2008/ols2008v1-pages-211-218.pdf).
>> >> Which I've been planning on trying to merge again due to a recent
>> >> resurgence of interest.  The device model for AXFS is... weird.  It
>> >> can use one or two devices at a time of any mix of NOR MTD, NAND MTD,
>> >> block, and unmanaged physical memory.  It's a terribly useful model
>> >> for embedded.  Anyway AXFS is readonly so hacking in a read only
>> >> dax_fault_nodev() and dax_file_read() would work fine, looks easy
>> >> enough.  But... it would be cool if similar small embedded focused RW
>> >> filesystems were enabled.
>> >
>> > Are those also out of tree?
>>
>> Of course.  Merging embedded filesystems is little merging regular
>> filesystems except 98% of you reviewers don't want it merged.
>
> You should at least be able to get it into staging these days.  I mean,
> look at some of the junk that's in staging ... and I don't think AXFS was
> nearly as bad.

Thanks....? ;)

>> IMO you're making DAX more complex by overly coupling to the bdev and
>> I think it could bite you later.  I submit this rework of the radix
>> tree and confusion about where to get the real bdev as evidence.  I'm
>> guessing that it won't be the last time.  It's unnecessary to couple
>> it like this, and in fact is not how the vfs has been layered in the
>> past.
>
> Huh?  The rework to use the radix tree for PFNs was done with one eye
> firmly on your usage case.  Just because I had to thread the get_block
> interface through it for the moment doesn't mean that I didn't have
> the "how do we get rid of get_block entirely" question on my mind.

Oh yeah.  I think we're on the same page.  But I'm not sure Dan is.  I
get the need to phase this in too.

> Using get_block seemed like the right idea three years ago.  I didn't
> know just how fundamentally ext4 and XFS disagree on how it should be
> used.

Sure.  I can see that.

>> To look at the the downside consider dax_fault().  Its called on a
>> fault to a user memory map, uses the filesystems get_block() to lookup
>> a sector so you can ask a block device to convert it to an address on
>> a DIMM.  Come on, that's awkward.  Everything around dax_fault() is
>> dripping with memory semantic interfaces, the dax_fault() call are
>> fundamentally about memory, the pmem calls are memory, the hardware is
>> memory, and yet it directly calls bdev_direct_access().  It's out of
>> place.
>
> What was out of place was the old 'get_xip_mem' in address_space
> operations.  Returning a kernel virtual address and a PFN from a
> filesystem operation?  That looks awful.

Yes.  Yes it does!  But at least my big hack was just one line. ;)
Nobody really even seemed to notice at the time.

>  All the other operations deal
> in struct pages, file offsets and occasionally sectors.  Of course, we
> don't have a struct page, so a pfn makes sense, but the kernel virtual
> address being returned was a gargantuan layering problem.

Well yes, but it was an expedient hack.

>> The legacy vfs/mm code didn't have this layering problem either.  Even
>> filemap_fault() that dax_fault() is modeled after doesn't call any
>> bdev methods directly, when it needs something it asks the filesystem
>> with a ->readpage().  The precedence is that you ask the filesystem
>> for what you need.  Look at the get_bdev() thing you've concluded you
>> need.  It _almost_ makes my point.  I just happen to be of the opinion
>> that you don't actually want or need the bdev, you want the pfn/kaddr
>> so you can flush or map or memcpy().
>
> You want the pfn.  The device driver doesn't have enough information to
> give you a (coherent with userspace) kaddr.  That's what (some future
> arch-specific implementation of) dax_map_pfn() is for.  That's why it
> takes 'index' as a parameter, so you can calculate where it'll be mapped
> in userspace, and determine an appropriate kernel virtual address to
> use for it.

Oh.... I think I'm just beginning to catch your vision for
dax_map_pfn().  I still don't get why we can't just do semi-arch
specific flushing instead of the alignment thing.  But that just might
be epic ignorance on my part.  Either way flush or magic alignments
dax_(un)map_pfn() would handle it, right?

[toc] | [prev] | [next] | [standalone]

#1323989

On Tue 02-02-16 08:47:30, Dave Chinner wrote:
> On Mon, Feb 01, 2016 at 03:51:47PM +0100, Jan Kara wrote:
> > On Sat 30-01-16 00:28:33, Matthew Wilcox wrote:
> > > On Fri, Jan 29, 2016 at 11:28:15AM -0700, Ross Zwisler wrote:
> > > > I guess I need to go off and understand if we can have DAX mappings on such a
> > > > device.  If we can, we may have a problem - we can get the block_device from
> > > > get_block() in I/O path and the various fault paths, but we don't have access
> > > > to get_block() when flushing via dax_writeback_mapping_range().  We avoid
> > > > needing it the normal case by storing the sector results from get_block() in
> > > > the radix tree.
> > > 
> > > I think we're doing it wrong by storing the sector in the radix tree; we'd
> > > really need to store both the sector and the bdev which is too much data.
> > > 
> > > If we store the PFN of the underlying page instead, we don't have this
> > > problem.  Instead, we have a different problem; of the device going
> > > away under us.  I'm trying to find the code which tears down PTEs when
> > > the device goes away, and I'm not seeing it.  What do we do about user
> > > mappings of the device?
> > 
> > So I don't have a strong opinion whether storing PFN or sector is better.
> > Maybe PFN is somewhat more generic but OTOH turning DAX off for special
> > cases like inodes on XFS RT devices would be IMHO fine.
> 
> We need to support alternate devices.
> 
> There is a strong case for using the XFS RT device with DAX,
> especially for applications that know they are going to always use
> large/huge/giant pages to access their data files. The XFS RT device
> can guarantee allocation is always aligned to large/huge/giant page
> constraints right up to ENOSPC and throughout the production life of
> the filesystem. We have no other filesystem capable of providing
> such guarantees, which means the XFS RT device is uniquely suited to
> certain aplications with DAX...

I see, thanks for explanation. So I'm OK with changing what is stored in
the radix tree to accommodate this use case but my reservation that we IHMO
have other more pressing things to fix remains...

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

[toc] | [prev] | [next] | [standalone]

#1324262

On Tue, Feb 2, 2016 at 3:17 AM, Jan Kara <jack@suse.cz> wrote:
[..]
> I see, thanks for explanation. So I'm OK with changing what is stored in
> the radix tree to accommodate this use case but my reservation that we IHMO
> have other more pressing things to fix remains...

We don't need pfns in the radix to support XFS RT configurations.
Just call get_blocks() again and use the sector, or am I missing
something?

[toc] | [prev] | [next] | [standalone]

#1324271

On Tue 02-02-16 08:33:56, Dan Williams wrote:
> On Tue, Feb 2, 2016 at 3:17 AM, Jan Kara <jack@suse.cz> wrote:
> [..]
> > I see, thanks for explanation. So I'm OK with changing what is stored in
> > the radix tree to accommodate this use case but my reservation that we IHMO
> > have other more pressing things to fix remains...
> 
> We don't need pfns in the radix to support XFS RT configurations.
> Just call get_blocks() again and use the sector, or am I missing
> something?

You are correct. But if you decide to pay the cost of additional
get_block() call, you only need the dirty tag in the radix tree and nothing
else. So my understanding was that the whole point of games with radix tree
is avoiding this extra get_block() calls for fsync().

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

[toc] | [prev] | [next] | [standalone]

#1324294

On Tue, Feb 2, 2016 at 8:46 AM, Jan Kara <jack@suse.cz> wrote:
> On Tue 02-02-16 08:33:56, Dan Williams wrote:
>> On Tue, Feb 2, 2016 at 3:17 AM, Jan Kara <jack@suse.cz> wrote:
>> [..]
>> > I see, thanks for explanation. So I'm OK with changing what is stored in
>> > the radix tree to accommodate this use case but my reservation that we IHMO
>> > have other more pressing things to fix remains...
>>
>> We don't need pfns in the radix to support XFS RT configurations.
>> Just call get_blocks() again and use the sector, or am I missing
>> something?
>
> You are correct. But if you decide to pay the cost of additional
> get_block() call, you only need the dirty tag in the radix tree and nothing
> else. So my understanding was that the whole point of games with radix tree
> is avoiding this extra get_block() calls for fsync().
>

DAX-fsync() is already a potentially expensive operation to cover data
durability guarantees for DAX-unaware applications.  A DAX-aware
application is going to skip fsync, and the get_blocks() cost, to do
cache management itself.

Willy pointed out some other potential benefits, assuming a suitable
replacement for the protections afforded by the block-device
percpu_ref counter can be found.  However, optimizing for the
DAX-unaware-application case seems the wrong motivation to me.

[toc] | [prev] | [next] | [standalone]

#1324329

On Tue, Feb 02, 2016 at 09:10:24AM -0800, Dan Williams wrote:
> On Tue, Feb 2, 2016 at 8:46 AM, Jan Kara <jack@suse.cz> wrote:
> > On Tue 02-02-16 08:33:56, Dan Williams wrote:
> >> On Tue, Feb 2, 2016 at 3:17 AM, Jan Kara <jack@suse.cz> wrote:
> >> [..]
> >> > I see, thanks for explanation. So I'm OK with changing what is stored in
> >> > the radix tree to accommodate this use case but my reservation that we IHMO
> >> > have other more pressing things to fix remains...
> >>
> >> We don't need pfns in the radix to support XFS RT configurations.
> >> Just call get_blocks() again and use the sector, or am I missing
> >> something?
> >
> > You are correct. But if you decide to pay the cost of additional
> > get_block() call, you only need the dirty tag in the radix tree and nothing
> > else. So my understanding was that the whole point of games with radix tree
> > is avoiding this extra get_block() calls for fsync().
> >
> 
> DAX-fsync() is already a potentially expensive operation to cover data
> durability guarantees for DAX-unaware applications.  A DAX-aware
> application is going to skip fsync, and the get_blocks() cost, to do
> cache management itself.
> 
> Willy pointed out some other potential benefits, assuming a suitable
> replacement for the protections afforded by the block-device
> percpu_ref counter can be found.  However, optimizing for the
> DAX-unaware-application case seems the wrong motivation to me.

Oh, no, the primary issue with calling get_block() in the fsync path isn't
performance.  It's that we don't have any idea what get_block() function to
call.

The fault handler calls all come from the filesystem directly, so they can
easily give us an appropriate get_block() function pointer.  But the
dax_writeback_mapping_range() calls come from the generic code in
mm/filemap.c, and don't know what get_block() to pass in.

During one iteration I had the calls to dax_writeback_mapping_range()
happening in the filesystem fsync code so that it could pass in get_block(),
but Dave Chinner pointed out that this misses other paths in the filesystem
that need to have things flushed via a call to filemap_write_and_wait_range().

In yet another iteration of this series I tried adding get_block() to struct
inode_operations so that I could access it from what is now
dax_writeback_mapping_range(), but this was shot down as well.

[toc] | [prev] | [next] | [standalone]

#1324332

On Tue, Feb 2, 2016 at 9:46 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Tue, Feb 2, 2016 at 9:34 AM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
>> On Tue, Feb 02, 2016 at 09:10:24AM -0800, Dan Williams wrote:
>>> On Tue, Feb 2, 2016 at 8:46 AM, Jan Kara <jack@suse.cz> wrote:
>>> > On Tue 02-02-16 08:33:56, Dan Williams wrote:
>>> >> On Tue, Feb 2, 2016 at 3:17 AM, Jan Kara <jack@suse.cz> wrote:
>>> >> [..]
>>> >> > I see, thanks for explanation. So I'm OK with changing what is stored in
>>> >> > the radix tree to accommodate this use case but my reservation that we IHMO
>>> >> > have other more pressing things to fix remains...
>>> >>
>>> >> We don't need pfns in the radix to support XFS RT configurations.
>>> >> Just call get_blocks() again and use the sector, or am I missing
>>> >> something?
>>> >
>>> > You are correct. But if you decide to pay the cost of additional
>>> > get_block() call, you only need the dirty tag in the radix tree and nothing
>>> > else. So my understanding was that the whole point of games with radix tree
>>> > is avoiding this extra get_block() calls for fsync().
>>> >
>>>
>>> DAX-fsync() is already a potentially expensive operation to cover data
>>> durability guarantees for DAX-unaware applications.  A DAX-aware
>>> application is going to skip fsync, and the get_blocks() cost, to do
>>> cache management itself.
>>>
>>> Willy pointed out some other potential benefits, assuming a suitable
>>> replacement for the protections afforded by the block-device
>>> percpu_ref counter can be found.  However, optimizing for the
>>> DAX-unaware-application case seems the wrong motivation to me.
>>
>> Oh, no, the primary issue with calling get_block() in the fsync path isn't
>> performance.  It's that we don't have any idea what get_block() function to
>> call.
>>
>> The fault handler calls all come from the filesystem directly, so they can
>> easily give us an appropriate get_block() function pointer.  But the
>> dax_writeback_mapping_range() calls come from the generic code in
>> mm/filemap.c, and don't know what get_block() to pass in.
>>
>> During one iteration I had the calls to dax_writeback_mapping_range()
>> happening in the filesystem fsync code so that it could pass in get_block(),
>> but Dave Chinner pointed out that this misses other paths in the filesystem
>> that need to have things flushed via a call to filemap_write_and_wait_range().
>>
>> In yet another iteration of this series I tried adding get_block() to struct
>> inode_operations so that I could access it from what is now
>> dax_writeback_mapping_range(), but this was shot down as well.
>
> Ugh, and we can't trigger it from where a filesystem normally syncs a
> block device, becauDid you tryse we lose track of the inode radix

[ sorry, copy paste error ]

block device, because we lose track of the inode radix

> information at that level.
>
> What a about a super_operation?  That seems the right level, given
> we're currently doing:
>
> inode->i_sb->s_bdev
>
> ...it does not seem terrible to instead do:
>
> inode->i_sb->s_op->get_block()

[toc] | [prev] | [next] | [standalone]

#1324395

On Tue, Feb 02, 2016 at 09:47:37AM -0800, Dan Williams wrote:
> On Tue, Feb 2, 2016 at 9:46 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> > On Tue, Feb 2, 2016 at 9:34 AM, Ross Zwisler
> > <ross.zwisler@linux.intel.com> wrote:
> >> On Tue, Feb 02, 2016 at 09:10:24AM -0800, Dan Williams wrote:
> >>> On Tue, Feb 2, 2016 at 8:46 AM, Jan Kara <jack@suse.cz> wrote:
> >>> > On Tue 02-02-16 08:33:56, Dan Williams wrote:
> >>> >> On Tue, Feb 2, 2016 at 3:17 AM, Jan Kara <jack@suse.cz> wrote:
> >>> >> [..]
> >>> >> > I see, thanks for explanation. So I'm OK with changing what is stored in
> >>> >> > the radix tree to accommodate this use case but my reservation that we IHMO
> >>> >> > have other more pressing things to fix remains...
> >>> >>
> >>> >> We don't need pfns in the radix to support XFS RT configurations.
> >>> >> Just call get_blocks() again and use the sector, or am I missing
> >>> >> something?
> >>> >
> >>> > You are correct. But if you decide to pay the cost of additional
> >>> > get_block() call, you only need the dirty tag in the radix tree and nothing
> >>> > else. So my understanding was that the whole point of games with radix tree
> >>> > is avoiding this extra get_block() calls for fsync().
> >>> >
> >>>
> >>> DAX-fsync() is already a potentially expensive operation to cover data
> >>> durability guarantees for DAX-unaware applications.  A DAX-aware
> >>> application is going to skip fsync, and the get_blocks() cost, to do
> >>> cache management itself.
> >>>
> >>> Willy pointed out some other potential benefits, assuming a suitable
> >>> replacement for the protections afforded by the block-device
> >>> percpu_ref counter can be found.  However, optimizing for the
> >>> DAX-unaware-application case seems the wrong motivation to me.
> >>
> >> Oh, no, the primary issue with calling get_block() in the fsync path isn't
> >> performance.  It's that we don't have any idea what get_block() function to
> >> call.
> >>
> >> The fault handler calls all come from the filesystem directly, so they can
> >> easily give us an appropriate get_block() function pointer.  But the
> >> dax_writeback_mapping_range() calls come from the generic code in
> >> mm/filemap.c, and don't know what get_block() to pass in.
> >>
> >> During one iteration I had the calls to dax_writeback_mapping_range()
> >> happening in the filesystem fsync code so that it could pass in get_block(),
> >> but Dave Chinner pointed out that this misses other paths in the filesystem
> >> that need to have things flushed via a call to filemap_write_and_wait_range().
> >>
> >> In yet another iteration of this series I tried adding get_block() to struct
> >> inode_operations so that I could access it from what is now
> >> dax_writeback_mapping_range(), but this was shot down as well.
> >
> > Ugh, and we can't trigger it from where a filesystem normally syncs a
> > block device, becauDid you tryse we lose track of the inode radix
> 
> [ sorry, copy paste error ]
> 
> block device, because we lose track of the inode radix
> 
> > information at that level.
> >
> > What a about a super_operation?  That seems the right level, given
> > we're currently doing:
> >
> > inode->i_sb->s_bdev
> >
> > ...it does not seem terrible to instead do:
> >
> > inode->i_sb->s_op->get_block()

This seems promising.  I'll try and code it up.

[toc] | [prev] | [next] | [standalone]

#1324335

On Tue, Feb 2, 2016 at 9:34 AM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Tue, Feb 02, 2016 at 09:10:24AM -0800, Dan Williams wrote:
>> On Tue, Feb 2, 2016 at 8:46 AM, Jan Kara <jack@suse.cz> wrote:
>> > On Tue 02-02-16 08:33:56, Dan Williams wrote:
>> >> On Tue, Feb 2, 2016 at 3:17 AM, Jan Kara <jack@suse.cz> wrote:
>> >> [..]
>> >> > I see, thanks for explanation. So I'm OK with changing what is stored in
>> >> > the radix tree to accommodate this use case but my reservation that we IHMO
>> >> > have other more pressing things to fix remains...
>> >>
>> >> We don't need pfns in the radix to support XFS RT configurations.
>> >> Just call get_blocks() again and use the sector, or am I missing
>> >> something?
>> >
>> > You are correct. But if you decide to pay the cost of additional
>> > get_block() call, you only need the dirty tag in the radix tree and nothing
>> > else. So my understanding was that the whole point of games with radix tree
>> > is avoiding this extra get_block() calls for fsync().
>> >
>>
>> DAX-fsync() is already a potentially expensive operation to cover data
>> durability guarantees for DAX-unaware applications.  A DAX-aware
>> application is going to skip fsync, and the get_blocks() cost, to do
>> cache management itself.
>>
>> Willy pointed out some other potential benefits, assuming a suitable
>> replacement for the protections afforded by the block-device
>> percpu_ref counter can be found.  However, optimizing for the
>> DAX-unaware-application case seems the wrong motivation to me.
>
> Oh, no, the primary issue with calling get_block() in the fsync path isn't
> performance.  It's that we don't have any idea what get_block() function to
> call.
>
> The fault handler calls all come from the filesystem directly, so they can
> easily give us an appropriate get_block() function pointer.  But the
> dax_writeback_mapping_range() calls come from the generic code in
> mm/filemap.c, and don't know what get_block() to pass in.
>
> During one iteration I had the calls to dax_writeback_mapping_range()
> happening in the filesystem fsync code so that it could pass in get_block(),
> but Dave Chinner pointed out that this misses other paths in the filesystem
> that need to have things flushed via a call to filemap_write_and_wait_range().
>
> In yet another iteration of this series I tried adding get_block() to struct
> inode_operations so that I could access it from what is now
> dax_writeback_mapping_range(), but this was shot down as well.

Ugh, and we can't trigger it from where a filesystem normally syncs a
block device, becauDid you tryse we lose track of the inode radix
information at that level.

What a about a super_operation?  That seems the right level, given
we're currently doing:

inode->i_sb->s_bdev

...it does not seem terrible to instead do:

inode->i_sb->s_op->get_block()

[toc] | [prev] | [next] | [standalone]

#1324477

On Tue, Feb 02, 2016 at 09:46:21AM -0800, Dan Williams wrote:
> What a about a super_operation?  That seems the right level, given
> we're currently doing:
> 
> inode->i_sb->s_bdev
> 
> ...it does not seem terrible to instead do:
> 
> inode->i_sb->s_op->get_block()

The point is that filesystems have lots of different get_block operations,
and the right one to use depends not just on the inode, but also upon
what VFS function is being called, and in some filesystems the phase
of the moon, or the file open flags (so even inode->i_ops->get_block is
wrong; file->f_ops->get_block would be better, but of course we've lost
that by the point we're doing writeback).

I now realise that basing DAX around get_block & buffer_heads was a mistake.
I think the Right Solution (not for 4.5) is to ask filesystems to populate
the radix tree.  A flow somewhat like this:

1. VFS or VM calls filesystem (eg ->fault())
2. Filesystem calls DAX (eg dax_fault())
3. DAX looks in radix tree, finds no information.
4. DAX calls (NEW!) mapping->a_ops->populate_pfns
5. Filesystem looks up its internal data structure (eg extent tree) and
   calls dax_create_pfns() (see giant patch from yesterday, only instead of
   passing a get_block_t, the filesystem has already filled in a bh which
   describes the entire extent that this access happens to land in).
6. DAX continues to take care of calling bdev_direct_access() from
   dax_create_pfns().

After we have that step done, we can look at what it would take to
avoid calling bdev_direct_access for non-block-based filesystems.
That looks to me like just calling dax_add_pfn_entries() from their
->populate_pfns implementation.  And we introduce a CONFIG_BLOCK ifdef
around dax_create_pfns(), dax_clear_blocks() and dax_zero_page_range().
Or maybe modify dax_zero_page_range() to use the radix tree as above,
since it's probably a useful helper function.

Once we have buffer_head usage confined to a fairly small part of DAX,
we can look at replacing it with a more appropriate data structure with
better-defined contents.

[toc] | [prev] | [next] | [standalone]

#1324524

On Tue, Feb 2, 2016 at 10:46 AM, Matthew Wilcox <willy@linux.intel.com> wrote:
> On Tue, Feb 02, 2016 at 09:46:21AM -0800, Dan Williams wrote:
>> What a about a super_operation?  That seems the right level, given
>> we're currently doing:
>>
>> inode->i_sb->s_bdev
>>
>> ...it does not seem terrible to instead do:
>>
>> inode->i_sb->s_op->get_block()
>
> The point is that filesystems have lots of different get_block operations,
> and the right one to use depends not just on the inode, but also upon
> what VFS function is being called, and in some filesystems the phase
> of the moon, or the file open flags (so even inode->i_ops->get_block is
> wrong; file->f_ops->get_block would be better, but of course we've lost
> that by the point we're doing writeback).

True, but in this case we're just trying to resolve the bdev for a
inode / sector combination to already allocated storage.  So
get_block() is a misnomer, this is just get_bdev() to resolve a
super_block-inode+sector tuple to a bdev for cases when s_bdev is the
wrong answer.

[toc] | [prev] | [next] | [standalone]

#1324588

On Tue, Feb 02, 2016 at 10:59:41AM -0800, Dan Williams wrote:
> True, but in this case we're just trying to resolve the bdev for a
> inode / sector combination to already allocated storage.  So
> get_block() is a misnomer, this is just get_bdev() to resolve a
> super_block-inode+sector tuple to a bdev for cases when s_bdev is the
> wrong answer.

Then let's call it that.  i_ops->get_bdev(struct inode *inode, pgoff_t index)
And if it doesn't exist, use i_sb->s_bdev.

[toc] | [prev] | [next] | [standalone]

Page 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

Back to top | Article view | linux.kernel

csiph-web