Re: linux-next test error

[Souptick Joarder]

On Thu, Sep 6, 2018 at 2:08 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Thu 06-09-18 00:54:50, Souptick Joarder wrote:
> > On Wed, Sep 5, 2018 at 7:05 PM Theodore Y. Ts'o <tytso@xxxxxxx> wrote:
> > >
> > > On Wed, Sep 05, 2018 at 03:20:16PM +0530, Souptick Joarder wrote:
> > > >
> > > > "fs: convert return type int to vm_fault_t" is still under
> > > > review/discusson and not yet merge
> > > > into linux-next. I am not seeing it into linux-next tree.Can you
> > > > please share the commit id ?
> > >
> > > It's at: 83c0adddcc6ed128168e7b87eaed0c21eac908e4 in the Linux Next
> > > branch.
> > >
> > > Dmitry, can you try reverting this commit and see if it makes the
> > > problem go away?
> > >
> > > Souptick, can we just NACK this patch and completely drop it from all
> > > trees?
> >
> > Ok, I will correct it and post v3.
> >
> > >
> > > I think we need to be a *lot* more careful about this vm_fault_t patch
> > > thing.  If you can't be bothered to run xfstests, we need to introduce
> > > a new function which replaces block_page_mkwrite() --- and then let
> > > each file system try to convert over to it at their own pace, after
> > > they've done regression testing.
> > >
> > >                                                 - Ted
> >
> > Chris has his opinion,
> >
> > block_page_mkwrite is only called by ext4 and nilfs2 anyway, so
> > converting both callers over should not be a problem, as long as
> > it actually is done properly.
> >
> > Matthew's opinion in other mail thread -
> >
> > > +vm_fault_t block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
> > > +                      get_block_t get_block, int *err)
> >
> > I don't like returning both the errno and the vm_fault_t.  To me that's a
> > sign we need to rethink this interface.
> >
> > I have two suggestions.  First, we could allocate a new VM_FAULT_NOSPC
> > bit.  Second, we could repurpose one of the existing bits, such as
> > VM_FAULT_RETRY for this purpose.
> >
> > > -int ext4_page_mkwrite(struct vm_fault *vmf)
> > > +vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
> >
> > I also think perhaps we could start by _not_ converting block_page_mkwrite().
> > Just convert ext4_page_mkwrite(), and save converting block_page_mkwrite()
> > for later.
>

> Yes, I'd start with converting ext4_page_mkwrite() - that should be pretty
> straightforward - and we can leave block_page_mkwrite() as is for now. I
> don't think allocating other VM_FAULT_ codes is going to cut it as
> generally the filesystem may need to communicate different error codes back
> and you don't know in advance which are interesting.

Then I need to take care of ext4_page_mkwrite() and ext4_filemap_fault()
to migrate to use vm_fault_t return type. Everything else can be removed
from this patch and it will go as a separate patch.

As block_page_mkwrite() is getting called from 2 places in ext4 and nilfs and
both places fault handler code convert errno to VM_FAULT_CODE using
block_page_mkwrite_return(), is it required to migrate block_page_mkwrite()
to use vm_fault_t return type and further complicate the API or better to
leave this API in current state ??

>
> One solution for passing error codes we could use with vm_fault_t is a
> scheme similar to ERR_PTR. So we can store full error code in vm_fault_t
> and still have a plenty of space for the special VM_FAULT_ return codes...
> With that scheme converting block_page_mkwrite() would be trivial.
>
I didn't get this part. Any reference code will be helpful ?