On Fri, 2022-05-06 at 22:30 +0800, Xiubo Li wrote:
> This will help reduce possible deadlock while holding Fcr to use
> getattr for read case.
>
> Usually we shouldn't use getattr to fetch inline data after getting
> Fcr caps, because it can cause deadlock. The solution is try uniline
> the inline data when opening files, thanks David Howells' previous
> work on uninlining the inline data work.
>
> It was caused from one possible call path:
> ceph_filemap_fault()-->
> ceph_get_caps(Fcr);
> filemap_fault()-->
> do_sync_mmap_readahead()-->
> page_cache_ra_order()-->
> read_pages()-->
> aops->readahead()-->
> netfs_readahead()-->
> netfs_begin_read()-->
> netfs_rreq_submit_slice()-->
> netfs_read_from_server()-->
> netfs_ops->issue_read()-->
> ceph_netfs_issue_read()-->
> ceph_netfs_issue_op_inline()-->
> getattr()
> ceph_pu_caps_ref(Fcr);
>
> This because if the Locker state is LOCK_EXEC_MIX for auth MDS, and
> the replica MDSes' lock state is LOCK_LOCK. Then the kclient could
> get 'Frwcb' caps from both auth and replica MDSes.
>
> But if the getattr is sent to any MDS, the MDS needs to do Locker
> transition to LOCK_MIX first and then to LOCK_SYNC. But when
> transfering to LOCK_MIX state the MDS Locker need to revoke the Fcb
> caps back, but the kclient already holding it and waiting the MDS
> to finish.
>
> Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> ---
> fs/ceph/file.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index 8c8226c0feac..09327ef5a26d 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -241,11 +241,12 @@ static int ceph_init_file_info(struct inode *inode, struct file *file,
> INIT_LIST_HEAD(&fi->rw_contexts);
> fi->filp_gen = READ_ONCE(ceph_inode_to_client(inode)->filp_gen);
>
> - if ((file->f_mode & FMODE_WRITE) &&
> - ci->i_inline_version != CEPH_INLINE_NONE) {
> - ret = ceph_uninline_data(file);
> - if (ret < 0)
> - goto error;
> + if (ci->i_inline_version != CEPH_INLINE_NONE) {
> + if (!ceph_pool_perm_check(inode, CEPH_CAP_FILE_WR)) {
> + ret = ceph_uninline_data(file);
> + if (ret < 0)
> + goto error;
> + }
> }
>
> return 0;
Note that this may be considered a regression in some cases. If a client
doesn't have write permissions to the pool at all, then previously it'd
be able to read files, but now it wouldn't.
Those are not terribly common configurations as I understand, but we
should be aware of it if we go this route.
Beyond that though, this patch ignores errors from ceph_pool_perm_check
and doesn't uninline the file if one is returned. Is that the behavior
you want here?
--
Jeff Layton <jlayton@xxxxxxxxxx>
