Re: [PATCH] aio: add aio_migratepage_helper to get rid of duplicate code

[Rakesh Pandit <rakesh@tuxera.com>]

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 <rakesh@tuxera.com>
> > ---
> >  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