On Wed, 2019-07-10 at 18:49 +0100, Luis Henriques wrote:
> Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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,
--
Jeff Layton <jlayton@xxxxxxxxxx>
