Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752796AbaBJRK4 (ORCPT ); Mon, 10 Feb 2014 12:10:56 -0500 Received: from nbl-ex10-fe01.nebula.fi ([188.117.32.121]:55337 "EHLO ex10.nebula.fi" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752133AbaBJRKy (ORCPT ); Mon, 10 Feb 2014 12:10:54 -0500 Date: Mon, 10 Feb 2014 19:10:45 +0200 From: Rakesh Pandit To: Benjamin LaHaise CC: , , , Subject: Re: [PATCH] aio: add aio_migratepage_helper to get rid of duplicate code Message-ID: <20140210171045.GA21031@localhost.localdomain> References: <20140210115255.GA3399@localhost.localdomain> <20140210162402.GB26657@kvack.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20140210162402.GB26657@kvack.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-Originating-IP: [194.100.106.164] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Feb 10, 2014 at 11:24:02AM -0500, Benjamin LaHaise wrote: > On Mon, Feb 10, 2014 at 01:52:56PM +0200, Rakesh Pandit wrote: > > You're missing a commit message here providing justification for the changes > submitted. Why is this change a good thing? Did it fix any bugs? > It is minor code refactoring to get rid of some duplication of code inside aio_migratepage. Doesn't fix any issue. -- Best regards, Rakesh Pandit > > > Signed-off-by: Rakesh Pandit > > --- > > fs/aio.c | 55 +++++++++++++++++++++++-------------------------------- > > 1 file changed, 23 insertions(+), 32 deletions(-) > > > > diff --git a/fs/aio.c b/fs/aio.c > > index d3d55dc..20d690a 100644 > > --- a/fs/aio.c > > +++ b/fs/aio.c > > @@ -278,32 +278,46 @@ static int aio_set_page_dirty(struct page *page) > > } > > > > #if IS_ENABLED(CONFIG_MIGRATION) > > -static int aio_migratepage(struct address_space *mapping, struct page *new, > > - struct page *old, enum migrate_mode mode) > > +static int aio_migratepage_helper(struct address_space *mapping, > > + struct page *new, struct page *old, bool move) > > { > > struct kioctx *ctx; > > unsigned long flags; > > - int rc; > > - > > - rc = 0; > > + int rc = 0; > > > > - /* Make sure the old page hasn't already been changed */ > > + /* We can potentially race against kioctx teardown here. Use the > > + * address_space's private data lock to protect the mapping's > > + * private_data. > > + */ > > spin_lock(&mapping->private_lock); > > ctx = mapping->private_data; > > if (ctx) { > > pgoff_t idx; > > spin_lock_irqsave(&ctx->completion_lock, flags); > > + if (move) > > + migrate_page_copy(new, old); > > idx = old->index; > > if (idx < (pgoff_t)ctx->nr_pages) { > > if (ctx->ring_pages[idx] != old) > > rc = -EAGAIN; > > + else if (move) > > + ctx->ring_pages[idx] = new; > > } else > > rc = -EINVAL; > > spin_unlock_irqrestore(&ctx->completion_lock, flags); > > } else > > - rc = -EINVAL; > > + rc = move ? -EBUSY : -EINVAL; > > spin_unlock(&mapping->private_lock); > > + return rc; > > +} > > > > +static int aio_migratepage(struct address_space *mapping, struct page *new, > > + struct page *old, enum migrate_mode mode) > > +{ > > + int rc = 0; > > + > > + /* Make sure the old page hasn't already been changed */ > > + rc = aio_migratepage_helper(mapping, new, old, false); > > if (rc != 0) > > return rc; > > > > @@ -317,31 +331,8 @@ static int aio_migratepage(struct address_space *mapping, struct page *new, > > return rc; > > } > > > > - /* We can potentially race against kioctx teardown here. Use the > > - * address_space's private data lock to protect the mapping's > > - * private_data. > > - */ > > - spin_lock(&mapping->private_lock); > > - ctx = mapping->private_data; > > - if (ctx) { > > - pgoff_t idx; > > - spin_lock_irqsave(&ctx->completion_lock, flags); > > - migrate_page_copy(new, old); > > - idx = old->index; > > - if (idx < (pgoff_t)ctx->nr_pages) { > > - /* And only do the move if things haven't changed */ > > - if (ctx->ring_pages[idx] == old) > > - ctx->ring_pages[idx] = new; > > - else > > - rc = -EAGAIN; > > - } else > > - rc = -EINVAL; > > - spin_unlock_irqrestore(&ctx->completion_lock, flags); > > - } else > > - rc = -EBUSY; > > - spin_unlock(&mapping->private_lock); > > - > > - if (rc == MIGRATEPAGE_SUCCESS) > > + rc = aio_migratepage_helper(mapping, new, old, true); > > + if (rc == 0) > > put_page(old); > > else > > put_page(new); > > -- > > 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/