ceph: clear page dirty before invalidate page
diff mbox series

Message ID CA+eEYqX6OkHEF0AhQ5E7DbSF16So7W0wiff=2uhgm9dmtsQGjQ@mail.gmail.com
State New
Headers show
Series
  • ceph: clear page dirty before invalidate page
Related show

Commit Message

erqi chen July 23, 2019, 7:55 a.m. UTC
From: Erqi Chen <chenerqi@gmail.com>

clear_page_dirty_for_io(page) before mapping->a_ops->invalidatepage().
invalidatepage() clears page's private flag, if dirty flag is not
cleared, the page may cause BUG_ON failure in ceph_set_page_dirty().

Fixes: https://tracker.ceph.com/issues/40862
Signed-off-by: Erqi Chen chenerqi@gmail.com
---
 fs/ceph/addr.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

                        }
--
1.8.3.1

Comments

Jeff Layton July 23, 2019, 10:25 a.m. UTC | #1
On Tue, 2019-07-23 at 15:55 +0800, erqi chen wrote:
> From: Erqi Chen <chenerqi@gmail.com>
> 
> clear_page_dirty_for_io(page) before mapping->a_ops->invalidatepage().
> invalidatepage() clears page's private flag, if dirty flag is not
> cleared, the page may cause BUG_ON failure in ceph_set_page_dirty().
> 
> Fixes: https://tracker.ceph.com/issues/40862
> Signed-off-by: Erqi Chen chenerqi@gmail.com
> ---
>  fs/ceph/addr.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index e078cc5..5ad63bf 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -914,9 +914,11 @@ static int ceph_writepages_start(struct
> address_space *mapping,
>                                 dout("%p page eof %llu\n",
>                                      page, ceph_wbc.i_size);
>                                 if (ceph_wbc.size_stable ||
> -                                   page_offset(page) >= i_size_read(inode))
> -                                       mapping->a_ops->invalidatepage(page,
> -                                                               0, PAGE_SIZE);
> +                                   page_offset(page) >= i_size_read(inode)) {
> +                                   if (clear_page_dirty_for_io(page))
> +                                       mapping->a_ops->invalidatepage(page,
> +                                                               0, PAGE_SIZE);
> +                               }
>                                 unlock_page(page);
>                                 continue;
>                         }
> --
> 1.8.3.1

Looks good. I'll plan to do a bit of testing with this today and then
merge it into the testing branch.

Thanks,
Jeff Layton July 23, 2019, 10:35 a.m. UTC | #2
On Tue, 2019-07-23 at 15:55 +0800, erqi chen wrote:
> From: Erqi Chen <chenerqi@gmail.com>
> 
> clear_page_dirty_for_io(page) before mapping->a_ops->invalidatepage().
> invalidatepage() clears page's private flag, if dirty flag is not
> cleared, the page may cause BUG_ON failure in ceph_set_page_dirty().
> 
> Fixes: https://tracker.ceph.com/issues/40862
> Signed-off-by: Erqi Chen chenerqi@gmail.com
> ---
>  fs/ceph/addr.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index e078cc5..5ad63bf 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -914,9 +914,11 @@ static int ceph_writepages_start(struct
> address_space *mapping,
>                                 dout("%p page eof %llu\n",
>                                      page, ceph_wbc.i_size);
>                                 if (ceph_wbc.size_stable ||
> -                                   page_offset(page) >= i_size_read(inode))
> -                                       mapping->a_ops->invalidatepage(page,
> -                                                               0, PAGE_SIZE);
> +                                   page_offset(page) >= i_size_read(inode)) {
> +                                   if (clear_page_dirty_for_io(page))
> +                                       mapping->a_ops->invalidatepage(page,
> +                                                               0, PAGE_SIZE);

It might be cleaner to just add:

    && clear_page_dirty_for_io(page)

to the existing if statement -- that will reduce the level of
indentation (which is already pretty far here).

> +                               }
>                                 unlock_page(page);
>                                 continue;
>                         }
> --
> 1.8.3.1

This patch looks good at first glance, but there is some whitespace
damage here and it does not apply. It looks like you may have cut and
pasted it into an email? Could you resend it? Maybe look into using
the git-send-email script to send it.

Thanks,
Jeff Layton July 24, 2019, 11:49 a.m. UTC | #3
On Tue, 2019-07-23 at 15:55 +0800, erqi chen wrote:
> From: Erqi Chen <chenerqi@gmail.com>
> 
> clear_page_dirty_for_io(page) before mapping->a_ops->invalidatepage().
> invalidatepage() clears page's private flag, if dirty flag is not
> cleared, the page may cause BUG_ON failure in ceph_set_page_dirty().
> 
> Fixes: https://tracker.ceph.com/issues/40862
> Signed-off-by: Erqi Chen chenerqi@gmail.com
> ---
>  fs/ceph/addr.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index e078cc5..5ad63bf 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -914,9 +914,11 @@ static int ceph_writepages_start(struct
> address_space *mapping,
>                                 dout("%p page eof %llu\n",
>                                      page, ceph_wbc.i_size);
>                                 if (ceph_wbc.size_stable ||
> -                                   page_offset(page) >= i_size_read(inode))
> -                                       mapping->a_ops->invalidatepage(page,
> -                                                               0, PAGE_SIZE);
> +                                   page_offset(page) >= i_size_read(inode)) {
> +                                   if (clear_page_dirty_for_io(page))
> +                                       mapping->a_ops->invalidatepage(page,
> +                                                               0, PAGE_SIZE);
> +                               }
>                                 unlock_page(page);
>                                 continue;
>                         }
> --
> 1.8.3.1

Thanks for the patch!

This one looks good. I'm running some tests and they look good so far. I
will plan to merge this into the testing branch later today assuming
there are no issues.

Patch
diff mbox series

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index e078cc5..5ad63bf 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -914,9 +914,11 @@  static int ceph_writepages_start(struct
address_space *mapping,
                                dout("%p page eof %llu\n",
                                     page, ceph_wbc.i_size);
                                if (ceph_wbc.size_stable ||
-                                   page_offset(page) >= i_size_read(inode))
-                                       mapping->a_ops->invalidatepage(page,
-                                                               0, PAGE_SIZE);
+                                   page_offset(page) >= i_size_read(inode)) {
+                                   if (clear_page_dirty_for_io(page))
+                                       mapping->a_ops->invalidatepage(page,
+                                                               0, PAGE_SIZE);
+                               }
                                unlock_page(page);
                                continue;