On Fri, 2017-08-04 at 11:51 +0800, Yan, Zheng wrote:
> ceph_readpage() unlocks page prematurely prematurely in the case
> that page is reading from fscache. Caller of readpage expects that
> page is uptodate when it get unlocked. So page shoule get locked
> by completion callback of fscache_read_or_alloc_pages()
>
> Signed-off-by: "Yan, Zheng" <zyan@xxxxxxxxxx>
> ---
> fs/ceph/addr.c | 26 +++++++++++++++++---------
> fs/ceph/cache.c | 12 +++---------
> 2 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 1b303f0..fc35627 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -189,7 +189,7 @@ static int ceph_releasepage(struct page *page, gfp_t g)
> /*
> * read a single page, without unlocking it.
> */
> -static int readpage_nounlock(struct file *filp, struct page *page)
> +static int ceph_do_readpage(struct file *filp, struct page *page)
> {
> struct inode *inode = file_inode(filp);
> struct ceph_inode_info *ci = ceph_inode(inode);
> @@ -218,8 +218,10 @@ static int readpage_nounlock(struct file *filp, struct page *page)
> }
>
> err = ceph_readpage_from_fscache(inode, page);
> - if (err == 0)
> + if (err == 0) {
> + err = -EINPROGRESS;
> goto out;
> + }
>
> dout("readpage inode %p file %p page %p index %lu\n",
> inode, filp, page, page->index);
> @@ -249,8 +251,11 @@ static int readpage_nounlock(struct file *filp, struct page *page)
>
> static int ceph_readpage(struct file *filp, struct page *page)
> {
> - int r = readpage_nounlock(filp, page);
> - unlock_page(page);
> + int r = ceph_do_readpage(filp, page);
> + if (r != -EINPROGRESS)
> + unlock_page(page);
> + else
> + r = 0;
> return r;
> }
>
> @@ -1216,7 +1221,7 @@ static int ceph_update_writeable_page(struct file *file,
> goto retry_locked;
> r = writepage_nounlock(page, NULL);
> if (r < 0)
> - goto fail_nosnap;
> + goto fail_unlock;
> goto retry_locked;
> }
>
> @@ -1244,11 +1249,14 @@ static int ceph_update_writeable_page(struct file *file,
> }
>
> /* we need to read it. */
> - r = readpage_nounlock(file, page);
> - if (r < 0)
> - goto fail_nosnap;
> + r = ceph_do_readpage(file, page);
> + if (r < 0) {
> + if (r == -EINPROGRESS)
> + return -EAGAIN;
> + goto fail_unlock;
> + }
> goto retry_locked;
> -fail_nosnap:
> +fail_unlock:
> unlock_page(page);
> return r;
> }
> diff --git a/fs/ceph/cache.c b/fs/ceph/cache.c
> index fd11728..337f886 100644
> --- a/fs/ceph/cache.c
> +++ b/fs/ceph/cache.c
> @@ -297,13 +297,7 @@ void ceph_fscache_file_set_cookie(struct inode *inode, struct file *filp)
> }
> }
>
> -static void ceph_vfs_readpage_complete(struct page *page, void *data, int error)
> -{
> - if (!error)
> - SetPageUptodate(page);
> -}
> -
> -static void ceph_vfs_readpage_complete_unlock(struct page *page, void *data, int error)
> +static void ceph_readpage_from_fscache_complete(struct page *page, void *data, int error)
> {
> if (!error)
> SetPageUptodate(page);
> @@ -331,7 +325,7 @@ int ceph_readpage_from_fscache(struct inode *inode, struct page *page)
> return -ENOBUFS;
>
> ret = fscache_read_or_alloc_page(ci->fscache, page,
> - ceph_vfs_readpage_complete, NULL,
> + ceph_readpage_from_fscache_complete, NULL,
> GFP_KERNEL);
>
> switch (ret) {
> @@ -360,7 +354,7 @@ int ceph_readpages_from_fscache(struct inode *inode,
> return -ENOBUFS;
>
> ret = fscache_read_or_alloc_pages(ci->fscache, mapping, pages, nr_pages,
> - ceph_vfs_readpage_complete_unlock,
> + ceph_readpage_from_fscache_complete,
> NULL, mapping_gfp_mask(mapping));
>
> switch (ret) {
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
