linux-ext4 - Re: [PATCH v3 2/2] ext4: handle layout changes to pinned DAX mappings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 5 Jul 2018 13:40:17 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     Dave Chinner <david@...morbit.com>, Jan Kara <jack@...e.cz>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        Christoph Hellwig <hch@....de>,
        linux-nvdimm <linux-nvdimm@...ts.01.org>,
        Jeff Moyer <jmoyer@...hat.com>,
        linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] ext4: handle layout changes to pinned DAX mappings

On Wed, Jul 4, 2018 at 8:59 PM, Darrick J. Wong <darrick.wong@...cle.com> wrote:
> On Thu, Jul 05, 2018 at 09:54:14AM +1000, Dave Chinner wrote:
>> On Wed, Jul 04, 2018 at 02:27:23PM +0200, Jan Kara wrote:
>> > On Wed 04-07-18 10:49:23, Dave Chinner wrote:
>> > > On Mon, Jul 02, 2018 at 11:29:12AM -0600, Ross Zwisler wrote:
>> > > > Follow the lead of xfs_break_dax_layouts() and add synchronization between
>> > > > operations in ext4 which remove blocks from an inode (hole punch, truncate
>> > > > down, etc.) and pages which are pinned due to DAX DMA operations.
>> > > >
>> > > > Signed-off-by: Ross Zwisler <ross.zwisler@...ux.intel.com>
>> > > > Reviewed-by: Jan Kara <jack@...e.cz>
>> > > > Reviewed-by: Lukas Czerner <lczerner@...hat.com>
>> > > > ---
>> > > >
>> > > > Changes since v2:
>> > > >  * Added a comment to ext4_insert_range() explaining why we don't call
>> > > >    ext4_break_layouts(). (Jan)
>> > >
>> > > Which I think is wrong and will cause data corruption.
>> > >
>> > > > @@ -5651,6 +5663,11 @@ int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len)
>> > > >                         LLONG_MAX);
>> > > >         if (ret)
>> > > >                 goto out_mmap;
>> > > > +       /*
>> > > > +        * We don't need to call ext4_break_layouts() because we aren't
>> > > > +        * removing any blocks from the inode.  We are just changing their
>> > > > +        * offset by inserting a hole.
>> > > > +        */
>
> Does calling ext4_break_layouts from insert range not work?
>
> It's my understanding that file leases work are a mechanism for the
> filesystem to delegate some of its authority over physical space
> mappings to "client" software.  AFAICT it's used for mmap'ing pmem
> directly into userspace and exporting space on shared storage over
> pNFS.  Some day we might use the same mechanism for the similar things
> that RDMA does, or the swapfile code since that's essentially how it
> works today.
>
> The other part of these file leases is that the filesystem revokes them
> any time it wants to perform a mapping operation on a file.  This breaks
> my mental model of how leases work, and if you commit to this for ext4
> then I'll have to remember that leases are different between xfs and
> ext4.  Worse, since the reason for skipping ext4_break_layouts seems to
> be the implementation detail that "DAX won't care", then someone else
> wiring up pNFS/future RDMA/whatever will also have to remember to put it
> back into ext4 or else kaboom.
>
> Granted, Dave said all these things already, but I actually feel
> strongly enough to reiterate.

This patch kit is only for the DAX fix, this isn't full layout lease
support. Even XFS is special casing unmap with the BREAK_UNMAP flag.
So ext4 is achieving feature parity for BREAK_UNMAP, just not
BREAK_WRITE, yet.