From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from bombadil.infradead.org ([198.137.202.133]:44446 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751860AbeDTUr5 (ORCPT ); Fri, 20 Apr 2018 16:47:57 -0400 Date: Fri, 20 Apr 2018 13:47:51 -0700 From: Matthew Wilcox To: Souptick Joarder Cc: mawilcox@microsoft.com, Ross Zwisler , Al Viro , linux-fsdevel Subject: Re: [PATCH] fs: dax: Adding new return type vm_fault_t Message-ID: <20180420204751.GG10788@bombadil.infradead.org> References: <20180417153507.GA31905@jordon-HP-15-Notebook-PC> <20180417210005.GC3603@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Sat, Apr 21, 2018 at 12:51:44AM +0530, Souptick Joarder wrote: > On Wed, Apr 18, 2018 at 2:30 AM, Matthew Wilcox 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.