| Message ID | 20190710161154.26125-3-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: fix races when uninlining data | expand |
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
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,
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); }
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(-)