2014-02-10 11:53:02

by Rakesh Pandit

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

Signed-off-by: Rakesh Pandit <[email protected]>
---
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

2014-02-10 16:24:08

by Benjamin LaHaise

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

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?

-ben

> Signed-off-by: Rakesh Pandit <[email protected]>
> ---
> 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

--
"Thought is the essence of where you are now."

2014-02-10 17:10:56

by Rakesh Pandit

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

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 <[email protected]>
> > ---
> > 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