linux-ext4 - Re: [PATCH 1/2] dax: Pass detailed error code from dax_iomap

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAPcyv4hJwaO+fgS+DKj4sqkXi2cP4-Qq5i+=c+_dfGKhZrZOuw@mail.gmail.com>
Date:   Tue, 2 Jan 2018 10:57:17 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     Jan Kara <jack@...e.cz>
Cc:     linux-ext4 <linux-ext4@...r.kernel.org>, Ted Tso <tytso@....edu>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-nvdimm@...ts.01.org
Subject: Re: [PATCH 1/2] dax: Pass detailed error code from dax_iomap_fault()

On Tue, Jan 2, 2018 at 10:54 AM, Jan Kara <jack@...e.cz> wrote:
> On Thu 21-12-17 09:12:52, Dan Williams wrote:
>> On Thu, Dec 21, 2017 at 8:30 AM, Jan Kara <jack@...e.cz> wrote:
>> > Ext4 needs to pass through error from its iomap handler to the page
>> > fault handler so that it can properly detect ENOSPC and force
>> > transaction commit and retry the fault (and block allocation). Add
>> > argument to dax_iomap_fault() for passing such error.
>> >
>> > Signed-off-by: Jan Kara <jack@...e.cz>
>> > ---
>> >  fs/dax.c            | 9 ++++++---
>> >  fs/ext2/file.c      | 2 +-
>> >  fs/ext4/file.c      | 2 +-
>> >  fs/xfs/xfs_file.c   | 2 +-
>> >  include/linux/dax.h | 2 +-
>> >  5 files changed, 10 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/fs/dax.c b/fs/dax.c
>> > index 95981591977a..f3afa1d6156c 100644
>> > --- a/fs/dax.c
>> > +++ b/fs/dax.c
>> > @@ -1096,7 +1096,7 @@ static bool dax_fault_is_synchronous(unsigned long flags,
>> >  }
>> >
>> >  static int dax_iomap_pte_fault(struct vm_fault *vmf, pfn_t *pfnp,
>> > -                              const struct iomap_ops *ops)
>> > +                              int *iomap_errp, const struct iomap_ops *ops)
>> >  {
>> >         struct vm_area_struct *vma = vmf->vma;
>> >         struct address_space *mapping = vma->vm_file->f_mapping;
>> > @@ -1149,6 +1149,8 @@ static int dax_iomap_pte_fault(struct vm_fault *vmf, pfn_t *pfnp,
>> >          * that we never have to deal with more than a single extent here.
>> >          */
>> >         error = ops->iomap_begin(inode, pos, PAGE_SIZE, flags, &iomap);
>> > +       if (iomap_errp)
>> > +               *iomap_errp = error;
>>
>> Since we already have 'struct iomap' tracking the state of the iomap
>> should we track the error status there as well?  I.e. move the on
>> stack allocation of struct iomap to the per-fs dax fault handlers.
>
> I don't think that's really adequate. Firstly because at least part of
> struct iomap needs to be initialized inside dax_iomap_fault() and secondly
> because by the time dax_iomap_fault() returns, mapping information in struct
> iomap may be invalid (as we unlocked radix tree entry). So I think it is
> really better to pass back only the error code and keep struct iomap
> internal to dax_iomap_fault().

Ok, that makes sense to me.