[2/3] ceph: pass unlocked page to ceph_uninline_data
diff mbox series

Message ID 20190710161154.26125-3-jlayton@kernel.org
State New
Headers show
Series
  • ceph: fix races when uninlining data
Related show

Commit Message

Jeff Layton July 10, 2019, 4:11 p.m. UTC
The only caller locks the page and then has to unlock it again once it
returns, just have ceph_uninline_data do that itself. Also, in the case
where we are allocating a fresh page for this, lock it to help simplify
the code a bit.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/addr.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

Comments

Luis Henriques July 10, 2019, 5:49 p.m. UTC | #1
Jeff Layton <jlayton@kernel.org> writes:

> The only caller locks the page and then has to unlock it again once it
> returns, just have ceph_uninline_data do that itself. Also, in the case
> where we are allocating a fresh page for this, lock it to help simplify
> the code a bit.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/addr.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 859d2cbfeccb..5f1e2b6577fb 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -1542,14 +1542,7 @@ static vm_fault_t ceph_page_mkwrite(struct vm_fault *vmf)
>  	ceph_block_sigs(&oldset);
>  
>  	if (ci->i_inline_version != CEPH_INLINE_NONE) {
> -		struct page *locked_page = NULL;
> -		if (off == 0) {
> -			lock_page(page);
> -			locked_page = page;
> -		}
> -		err = ceph_uninline_data(inode, locked_page);
> -		if (locked_page)
> -			unlock_page(locked_page);
> +		err = ceph_uninline_data(inode, off == 0 ? page : NULL);
>  		if (err < 0)
>  			goto out_free;
>  	}
> @@ -1663,12 +1656,11 @@ void ceph_fill_inline_data(struct inode *inode, struct page *locked_page,
>  	}
>  }
>  
> -int ceph_uninline_data(struct inode *inode, struct page *locked_page)
> +int ceph_uninline_data(struct inode *inode, struct page *page)
>  {
>  	struct ceph_inode_info *ci = ceph_inode(inode);
>  	struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
>  	struct ceph_osd_request *req;
> -	struct page *page = NULL;
>  	u64 len, inline_version;
>  	int err = 0;
>  	bool from_pagecache = false;
> @@ -1684,8 +1676,8 @@ int ceph_uninline_data(struct inode *inode, struct page *locked_page)
>  	    inline_version == CEPH_INLINE_NONE)
>  		goto out;
>  
> -	if (locked_page) {
> -		page = locked_page;
> +	if (page) {
> +		lock_page(page);
>  		WARN_ON(!PageUptodate(page));
>  	} else if (ceph_caps_issued(ci) &
>  		   (CEPH_CAP_FILE_CACHE|CEPH_CAP_FILE_LAZYIO)) {
> @@ -1711,6 +1703,7 @@ int ceph_uninline_data(struct inode *inode, struct page *locked_page)
>  			err = -ENOMEM;
>  			goto out;
>  		}
> +		lock_page(page);
>  		err = __ceph_do_getattr(inode, page,
>  					CEPH_STAT_CAP_INLINE_DATA, true);
>  		if (err < 0) {
> @@ -1782,11 +1775,11 @@ int ceph_uninline_data(struct inode *inode, struct page *locked_page)
>  	if (err == -ECANCELED)
>  		err = 0;
>  out:
> -	if (page && page != locked_page) {
> -		if (from_pagecache) {
> -			unlock_page(page);
> +	if (page) {
> +		unlock_page(page);
> +		if (from_pagecache)
>  			put_page(page);
> -		} else
> +		else
>  			__free_pages(page, 0);
                        ^^^^^^^^^^^^

If we get page as the function parameter we will end up freeing it here,
which isn't what we want.  We can probably keep a local page variable so
that we compare it with whatever we received as parameter.

Cheers
Jeff Layton July 10, 2019, 6:32 p.m. UTC | #2
On Wed, 2019-07-10 at 18:49 +0100, Luis Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:
> 
> > The only caller locks the page and then has to unlock it again once it
> > returns, just have ceph_uninline_data do that itself. Also, in the case
> > where we are allocating a fresh page for this, lock it to help simplify
> > the code a bit.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/addr.c | 25 +++++++++----------------
> >  1 file changed, 9 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > index 859d2cbfeccb..5f1e2b6577fb 100644
> > --- a/fs/ceph/addr.c
> > +++ b/fs/ceph/addr.c
> > @@ -1542,14 +1542,7 @@ static vm_fault_t ceph_page_mkwrite(struct vm_fault *vmf)
> >  	ceph_block_sigs(&oldset);
> >  
> >  	if (ci->i_inline_version != CEPH_INLINE_NONE) {
> > -		struct page *locked_page = NULL;
> > -		if (off == 0) {
> > -			lock_page(page);
> > -			locked_page = page;
> > -		}
> > -		err = ceph_uninline_data(inode, locked_page);
> > -		if (locked_page)
> > -			unlock_page(locked_page);
> > +		err = ceph_uninline_data(inode, off == 0 ? page : NULL);
> >  		if (err < 0)
> >  			goto out_free;
> >  	}
> > @@ -1663,12 +1656,11 @@ void ceph_fill_inline_data(struct inode *inode, struct page *locked_page,
> >  	}
> >  }
> >  
> > -int ceph_uninline_data(struct inode *inode, struct page *locked_page)
> > +int ceph_uninline_data(struct inode *inode, struct page *page)
> >  {
> >  	struct ceph_inode_info *ci = ceph_inode(inode);
> >  	struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
> >  	struct ceph_osd_request *req;
> > -	struct page *page = NULL;
> >  	u64 len, inline_version;
> >  	int err = 0;
> >  	bool from_pagecache = false;
> > @@ -1684,8 +1676,8 @@ int ceph_uninline_data(struct inode *inode, struct page *locked_page)
> >  	    inline_version == CEPH_INLINE_NONE)
> >  		goto out;
> >  
> > -	if (locked_page) {
> > -		page = locked_page;
> > +	if (page) {
> > +		lock_page(page);
> >  		WARN_ON(!PageUptodate(page));
> >  	} else if (ceph_caps_issued(ci) &
> >  		   (CEPH_CAP_FILE_CACHE|CEPH_CAP_FILE_LAZYIO)) {
> > @@ -1711,6 +1703,7 @@ int ceph_uninline_data(struct inode *inode, struct page *locked_page)
> >  			err = -ENOMEM;
> >  			goto out;
> >  		}
> > +		lock_page(page);
> >  		err = __ceph_do_getattr(inode, page,
> >  					CEPH_STAT_CAP_INLINE_DATA, true);
> >  		if (err < 0) {
> > @@ -1782,11 +1775,11 @@ int ceph_uninline_data(struct inode *inode, struct page *locked_page)
> >  	if (err == -ECANCELED)
> >  		err = 0;
> >  out:
> > -	if (page && page != locked_page) {
> > -		if (from_pagecache) {
> > -			unlock_page(page);
> > +	if (page) {
> > +		unlock_page(page);
> > +		if (from_pagecache)
> >  			put_page(page);
> > -		} else
> > +		else
> >  			__free_pages(page, 0);
>                         ^^^^^^^^^^^^
> 
> If we get page as the function parameter we will end up freeing it here,
> which isn't what we want.  We can probably keep a local page variable so
> that we compare it with whatever we received as parameter.
> 

Good catch. I'll fix that up.

Thanks,

Patch
diff mbox series

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 859d2cbfeccb..5f1e2b6577fb 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -1542,14 +1542,7 @@  static vm_fault_t ceph_page_mkwrite(struct vm_fault *vmf)
 	ceph_block_sigs(&oldset);
 
 	if (ci->i_inline_version != CEPH_INLINE_NONE) {
-		struct page *locked_page = NULL;
-		if (off == 0) {
-			lock_page(page);
-			locked_page = page;
-		}
-		err = ceph_uninline_data(inode, locked_page);
-		if (locked_page)
-			unlock_page(locked_page);
+		err = ceph_uninline_data(inode, off == 0 ? page : NULL);
 		if (err < 0)
 			goto out_free;
 	}
@@ -1663,12 +1656,11 @@  void ceph_fill_inline_data(struct inode *inode, struct page *locked_page,
 	}
 }
 
-int ceph_uninline_data(struct inode *inode, struct page *locked_page)
+int ceph_uninline_data(struct inode *inode, struct page *page)
 {
 	struct ceph_inode_info *ci = ceph_inode(inode);
 	struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
 	struct ceph_osd_request *req;
-	struct page *page = NULL;
 	u64 len, inline_version;
 	int err = 0;
 	bool from_pagecache = false;
@@ -1684,8 +1676,8 @@  int ceph_uninline_data(struct inode *inode, struct page *locked_page)
 	    inline_version == CEPH_INLINE_NONE)
 		goto out;
 
-	if (locked_page) {
-		page = locked_page;
+	if (page) {
+		lock_page(page);
 		WARN_ON(!PageUptodate(page));
 	} else if (ceph_caps_issued(ci) &
 		   (CEPH_CAP_FILE_CACHE|CEPH_CAP_FILE_LAZYIO)) {
@@ -1711,6 +1703,7 @@  int ceph_uninline_data(struct inode *inode, struct page *locked_page)
 			err = -ENOMEM;
 			goto out;
 		}
+		lock_page(page);
 		err = __ceph_do_getattr(inode, page,
 					CEPH_STAT_CAP_INLINE_DATA, true);
 		if (err < 0) {
@@ -1782,11 +1775,11 @@  int ceph_uninline_data(struct inode *inode, struct page *locked_page)
 	if (err == -ECANCELED)
 		err = 0;
 out:
-	if (page && page != locked_page) {
-		if (from_pagecache) {
-			unlock_page(page);
+	if (page) {
+		unlock_page(page);
+		if (from_pagecache)
 			put_page(page);
-		} else
+		else
 			__free_pages(page, 0);
 	}