Re: [PATCH] ceph: is_root_ceph_dentry() cleanup

[Al Viro <viro@zeniv.linux.org.uk>]

On Tue, Jan 28, 2025 at 11:27:05PM +0000, Viacheslav Dubeyko wrote:

> I assume that you imply this code:
> 
> 	/* can we conclude ENOENT locally? */
> 	if (d_really_is_negative(dentry)) {
> 		struct ceph_inode_info *ci = ceph_inode(dir);
> 		struct ceph_dentry_info *di = ceph_dentry(dentry);
> 
> 		spin_lock(&ci->i_ceph_lock);
> 		doutc(cl, " dir %llx.%llx flags are 0x%lx\n",
> 		      ceph_vinop(dir), ci->i_ceph_flags);
> 		if (strncmp(dentry->d_name.name,
> 			    fsc->mount_options->snapdir_name,
> 			    dentry->d_name.len) &&
> 		    !is_root_ceph_dentry(dir, dentry) &&
> 		    ceph_test_mount_opt(fsc, DCACHE) &&
> 		    __ceph_dir_is_complete(ci) &&
> 		    __ceph_caps_issued_mask_metric(ci, CEPH_CAP_FILE_SHARED,
> 1)) {
> 			__ceph_touch_fmode(ci, mdsc, CEPH_FILE_MODE_RD);
> 			spin_unlock(&ci->i_ceph_lock);
> 			doutc(cl, " dir %llx.%llx complete, -ENOENT\n",
> 			      ceph_vinop(dir));
> 			d_add(dentry, NULL);
> 			di->lease_shared_gen = atomic_read(&ci->i_shared_gen);
> 			return NULL;
> 		}
> 		spin_unlock(&ci->i_ceph_lock);
> 	}
> 
> Am I correct? So, how can we rework this code if it's wrong? What is your
> vision? Do you mean that it's dead code? How can we check it?

I mean that ->lookup() is called *ONLY* for a negative unhashed dentries.
In other words, on a call from VFS that condition will always be true.
That part is easily provable; what is harder to reason about is the
direct call of ceph_lookup() from ceph_handle_notrace_create().

The callers of that thing (ceph_mknod(), ceph_symlink() and ceph_mkdir())
are all guaranteed that dentry will be negative when they are called.
The hard-to-reason-about part is the call of ceph_mdsc_do_request()
directly preceding the calls of ceph_handle_notrace_create().

Can ceph_mdsc_do_request() return 0, with req->r_reply_info.head->is_dentry
being false *AND* a call of splice_dentry() made by ceph_fill_trace() called
by ceph_mdsc_do_request()?

AFAICS, there are 3 calls of splice_dentry(); two of them happen under
explicit check for ->is_dentry and thus are not interesting for our
purposes.  The third one, though, could be hit with ->is_dentry being
false and ->r_op being CEPH_MDS_OP_MKSNAP.  That is not impossible
from ceph_mkdir(), as far as I can tell, and I don't understand the
details well enough to tell whether it can actually happen.

Is it actually possible to hit ceph_handle_notrace_create() with
a positive dentry?