Re: [PATCH] fs: dax: Adding new return type vm_fault_t

[Matthew Wilcox <willy@infradead.org>]

On Sat, Apr 21, 2018 at 12:51:44AM +0530, Souptick Joarder wrote:
> On Wed, Apr 18, 2018 at 2:30 AM, Matthew Wilcox <willy@infradead.org> wrote:
> > On Tue, Apr 17, 2018 at 09:05:07PM +0530, Souptick Joarder wrote:
> >> There was an existing bug inside dax_load_hole()
> >> if vm_insert_mixed had failed to allocate a page table,
> >> we'd return VM_FAULT_NOPAGE instead of VM_FAULT_OOM.
> >> With vmf_insert_mixed this issue is addressed.
> >>
> >> vmf_insert_mixed_mkwrite() is the new wrapper function
> >> which will convert err returned from vm_insert_mixed_
> >> mkwrite() to vm_fault_t type.
> >
> > Since dax is the only caller of vm_insert_mixed_mkwrite(), rather
> > than introducing a wrapper function, you should just convert
> > vm_insert_mixed_mkwrite() to vmf_insert_mixed_mkwrite().
> >
> 
> Although vm_insert_mixed_mkwrite () is getting called only from dax,
> but if we directly change it to vmf_insert_mixed_mkwrite()
> with return type vm_fault_t, we end with changing multiple functions
> recursively. In my opinion, it will complicate things.
> 
> It's better to go with current inline vmf_insert_mixed_mkwrite() approach.

No, it's not.  The point is to create patches which are self-contained
and don't break anything.  So we can't change vm_insert_mixed because
it has so many users, and we need to transition to it gradually.
But vm_insert_mixed_mkwrite only has one user, and we can just change
both at the same time.