Re: [PATCH v5 4/5] ceph: record subvolume quarantine from MDS cap updates

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ceph: epoch self-invalidation in ceph_quarantine_reply_can_clear()
- [High] ceph: unhashed directory alias traversal in ceph_quarantine_walk_once()
- [High] ceph: pre-import cap sequence off-by-one in handle_cap_grant()
- [Medium] ceph: unconditional quarantine reset on non-reporting MDS sessions
--

commit 78e79694b7e226fb4224da6b24cf5abda7482a66
Author: Alex Markuze <amarkuze@redhat.com>

ceph: record subvolume quarantine from MDS cap updates

This commit decodes the MClientCaps v14 quarantine errno to maintain a
sticky, counted quarantine mark on inodes, and adds an epoch-based rcu
ancestry walk cache to track whether an inode is under a quarantined
subvolume.

> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index b27bcdc24d29f..94f015112942d 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -692,8 +692,9 @@ void change_auth_cap_ses(struct ceph_inode_info *ci,
>   * @fmode is the open file mode, if we are opening a file, otherwise
>   * it is < 0.  (This is so we can atomically add the cap and add an
>   * open file reference to it.)
> + * Returns false when the raw grant predates the recorded cap sequence.
>   */
> -void ceph_add_cap(struct inode *inode,
> +bool ceph_add_cap(struct inode *inode,
>  		  struct ceph_mds_session *session, u64 cap_id,
>  		  unsigned issued, unsigned wanted,
>  		  unsigned seq, unsigned mseq, u64 realmino, int flags,
> @@ -706,6 +707,7 @@ void ceph_add_cap(struct inode *inode,
>  	int mds = session->s_mds;
>  	int actual_wanted;
>  	u32 gen;
> +	bool cap_current = true;
>  
>  	lockdep_assert_held(&ci->i_ceph_lock);
>  
> @@ -737,6 +739,7 @@ void ceph_add_cap(struct inode *inode,
>  		atomic64_inc(&mdsc->metric.total_caps);
>  		spin_unlock(&session->s_cap_lock);
>  	} else {
> +		cap_current = ceph_seq_cmp(seq, cap->seq) >= 0;
>  		spin_lock(&session->s_cap_lock);
>  		list_move_tail(&cap->session_caps, &session->s_caps);
>  		spin_unlock(&session->s_cap_lock);
> @@ -823,6 +826,7 @@ void ceph_add_cap(struct inode *inode,
>  	cap->mseq = mseq;
>  	cap->cap_gen = gen;
>  	wake_up_all(&ci->i_cap_wq);
> +	return cap_current;
>  }

[ ... ]

> @@ -3757,6 +3769,45 @@ static int parse_inline_data(void **p, void *end,
>  	return -EIO;
>  }
>  
> +/*
> + * Record quarantine state carried by an MClientCaps oserrno.
> + * Caller holds i_ceph_lock. Returns true when waiters should be woken
> + * because quarantine changed or a pending reset was resolved.
> + *
> + * -EQUARANTINED refreshes the sticky flag and its reporting MDS rank.
> + * A current grant above PIN with errno 0 clears only that rank's mark.
> + * Another rank needs a successful metadata request for the current epoch.
> + * A PIN-only update may zero qtine_errno while leaving the flag set.
> + */
> +static bool ceph_apply_quarantine(struct ceph_inode_info *ci, int mds,
> +				  int newcaps, int oserrno)
> +{
> +	bool was, now, pending;
> +
> +	was = READ_ONCE(ci->i_qtine_errno) == -CEPH_EQUARANTINED ||
> +	      test_bit(CEPH_I_QUARANTINE_BIT, &ci->i_ceph_flags);
> +	pending = test_bit(CEPH_I_QUARANTINE_PENDING_BIT, &ci->i_ceph_flags);
> +
> +	if ((newcaps & ~CEPH_CAP_PIN) && was && oserrno == 0 &&
> +	    READ_ONCE(ci->i_qtine_mds) == mds)
> +		ceph_quarantine_clear(ci);
> +
> +	if (oserrno == -CEPH_EQUARANTINED) {
> +		ceph_quarantine_set(ci);
> +		WRITE_ONCE(ci->i_qtine_mds, mds);
> +	}
> +
> +	if (newcaps || was ||
> +	    test_bit(CEPH_I_QUARANTINE_BIT, &ci->i_ceph_flags))
> +		WRITE_ONCE(ci->i_qtine_errno, oserrno);
> +
> +	now = READ_ONCE(ci->i_qtine_errno) == -CEPH_EQUARANTINED ||
> +	      test_bit(CEPH_I_QUARANTINE_BIT, &ci->i_ceph_flags);
> +	return was != now ||
> +	       pending != test_bit(CEPH_I_QUARANTINE_PENDING_BIT,
> +				   &ci->i_ceph_flags);
> +}
> +
>  /*
>   * Handle a cap GRANT message from the MDS.  (Note that a GRANT may
>   * actually be a revocation if it specifies a smaller cap set.)
> @@ -3781,6 +3832,7 @@ static void handle_cap_grant(struct inode *inode,
>  	u64 max_size = le64_to_cpu(grant->max_size);
>  	unsigned char check_caps = 0;
>  	bool was_stale = cap->cap_gen < atomic_read(&session->s_cap_gen);
> +	bool quarantine_current = ceph_seq_cmp(seq, cap->seq) >= 0;
>  	bool wake = false;
>  	bool writeback = false;
>  	bool queue_trunc = false;
> @@ -4128,6 +4180,15 @@ static void handle_cap_grant(struct inode *inode,
>  			fill_inline = true;
>  	}
>  
> +	if (quarantine_current && extra_info->oserrno_valid &&
> +	    ceph_apply_quarantine(ci, session->s_mds, newcaps,
> +				  extra_info->oserrno)) {
> +		boutc(cl, "quarantine oserrno %d caps %s %llx.%llx\n",
> +		      extra_info->oserrno, ceph_cap_string(newcaps),
> +		      ceph_vinop(inode));
> +		wake = true;
> +	}

[Severity: High]
When an export target mds sends a cap message or reply just before cap
import, can ceph_seq_cmp(seq, cap->seq) >= 0 treat a pre-import sequence
number as current?

In fs/ceph/caps.c, when an export message arrives before the corresponding
import message, handle_cap_export() sets the target mds cap sequence to
t_issue_seq - 1 and merges the exported caps into cap->issued:

fs/ceph/caps.c:handle_cap_export() {
	if (tcap->cap_id == t_cap_id &&
	    ceph_seq_cmp(tcap->seq, t_issue_seq) < 0) {
		tcap->cap_id = t_cap_id;
		tcap->seq = t_issue_seq - 1;
		tcap->issue_seq = t_issue_seq - 1;
		tcap->issued |= issued;
		...
	}
	...
	} else if (tsession) {
		ceph_add_cap(inode, tsession, t_cap_id, issued, 0,
			     t_issue_seq - 1, t_mseq, (u64)-1, flag, &new_cap);
	}
}

Both ceph_add_cap() and handle_cap_grant() treat any message with
ceph_seq_cmp(seq, cap->seq) <= 0 (including seq == cap->seq, which is
t_issue_seq - 1) as a message sent before the cap import, and merge
cap->issued into issued or newcaps:

fs/ceph/caps.c:handle_cap_grant() {
	if (ceph_seq_cmp(seq, cap->seq) <= 0) {
		WARN_ON(cap != ci->i_auth_cap);
		WARN_ON(cap->cap_id != le64_to_cpu(grant->cap_id));
		seq = cap->seq;
		newcaps |= cap->issued;
	}
}

Because cap_current in ceph_add_cap() and quarantine_current in
handle_cap_grant() use ceph_seq_cmp(seq, cap->seq) >= 0, a pre-import
message with seq == t_issue_seq - 1 satisfies both <= 0 and >= 0. In
handle_cap_grant(), newcaps is inflated with cap->issued from the export
placeholder before ceph_apply_quarantine() checks (newcaps & ~CEPH_CAP_PIN),
and in ceph_add_cap(), cap_current returns true to ceph_fill_inode() for a
pre-import reply.

Should pre-import messages with seq == cap->seq (when the op is not
CEPH_CAP_OP_IMPORT) be excluded from updating or clearing quarantine state?

[ ... ]

> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 94f5215f8eed5..78e97cf2f9a41 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c

[ ... ]

> +/*
> + * Parent walk with no inode references. Caller holds rcu_read_lock().
> + * Stops at the mount root, at an MDS mark, or at an ancestor whose
> + * cached answer is for this epoch. No hashed alias, or the depth
> + * limit, is not an answer: the caller must not cache it.
> + */
> +static int ceph_quarantine_walk_once(struct inode *inode, u64 epoch,
> +				     bool use_cache)
> +{
> +	struct inode *cur = inode;
> +	unsigned int depth = 0;
> +	bool under = false;
> +	bool done = false;
> +
> +	while (cur && depth++ < 256) {
> +		struct ceph_inode_info *ci = ceph_inode(cur);
> +		struct dentry *alias;
> +		struct dentry *parent;
> +		struct inode *next;
> +
> +		if (test_bit(CEPH_I_QUARANTINE_PENDING_BIT, &ci->i_ceph_flags))
> +			break;
> +		if (READ_ONCE(ci->i_qtine_errno) == -CEPH_EQUARANTINED ||
> +		    test_bit(CEPH_I_QUARANTINE_BIT, &ci->i_ceph_flags)) {
> +			under = true;
> +			done = true;
> +			break;
> +		}
> +
> +		if (use_cache && cur != inode &&
> +		    ceph_qtag_read(ci, epoch, &under)) {
> +			done = true;
> +			break;
> +		}
> +
> +		alias = d_find_alias_rcu(cur);
> +		if (!alias)
> +			break;
> +
> +		parent = READ_ONCE(alias->d_parent);
> +		/*
> +		 * The mount root is its own parent. A disconnected
> +		 * alias from d_obtain_alias(), or a secondary root from
> +		 * d_obtain_root(), has the same shape and is not a
> +		 * finished walk.
> +		 */
> +		if (!parent || parent == alias) {
> +			if (parent == alias && S_ISDIR(cur->i_mode) &&
> +			    alias == READ_ONCE(inode->i_sb->s_root))
> +				done = true;
> +			break;
> +		}
> +
> +		next = d_inode_rcu(parent);

[Severity: High]
If a directory dentry is unhashed while still referenced, could
ceph_quarantine_walk_once() follow a stale d_parent chain to s_root and
cache a clear result in i_qtag?

Looking at d_find_alias_rcu() in fs/dcache.c, it only checks !d_unhashed()
for non-directory inodes, and returns the first alias unconditionally when
S_ISDIR(inode->i_mode) is true:

fs/dcache.c:d_find_alias_rcu() {
	if (likely(!(inode_state_read(inode) & I_FREEING) && !hlist_empty(l))) {
		if (S_ISDIR(inode->i_mode)) {
			de = hlist_entry(l->first, struct dentry, d_alias);
		} else {
			hlist_for_each_entry(de, l, d_alias)
				if (!d_unhashed(de))
					break;
		}
	}
	return de;
}

If a directory dentry is unhashed via d_drop(), d_delete(), or
d_invalidate() (for example, after a remote rename into a quarantined
subvolume) while held open or referenced by a child dentry, alias remains on
cur->i_dentry with d_unhashed(alias) true and alias->d_parent still pointing
to the old parent directory.

Since ceph_quarantine_walk_once() does not check d_unhashed(alias) when
parent != alias, wouldn't the walk continue up the old parent chain to
s_root and publish a clear tag in ci->i_qtag?

[ ... ]

> +void ceph_quarantine_splice_start(struct super_block *sb)
> +{
> +	struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(sb);
> +
> +	spin_lock(&mdsc->quarantine_lock);
> +	atomic_inc_return(&mdsc->quarantine_splices);
> +	atomic64_add_return(2, &mdsc->quarantine_epoch);
> +	spin_unlock(&mdsc->quarantine_lock);
> +}
> +
> +void ceph_quarantine_splice_finish(struct super_block *sb)
> +{
> +	struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(sb);
> +
> +	spin_lock(&mdsc->quarantine_lock);
> +	atomic64_add_return(2, &mdsc->quarantine_epoch);
> +	atomic_dec_return(&mdsc->quarantine_splices);
> +	spin_unlock(&mdsc->quarantine_lock);
> +}
> +
> +static bool ceph_quarantine_reply_can_clear(struct ceph_inode_info *ci,
> +					    struct ceph_mds_session *session)
> +{
> +	struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(ci->netfs.inode.i_sb);
> +	struct ceph_mds_request *req;
> +	u64 epoch;
> +
> +	if (READ_ONCE(ci->i_qtine_mds) == session->s_mds)
> +		return true;
> +	/* Different ranks have independent cap sequences. */
> +	req = ceph_current_mds_request(mdsc->fsc);
> +	epoch = (u64)atomic64_read_acquire(&mdsc->quarantine_epoch);
> +	return req && !(epoch & 1) && req->r_quarantine_epoch == epoch &&
> +		!atomic_read(&mdsc->quarantine_splices);
> +}

[ ... ]

> @@ -1407,7 +1866,7 @@ int ceph_fill_inode(struct inode *inode, struct folio *locked_folio,
>  	/* were we issued a capability? */
>  	if (info_caps) {
>  		if (!ceph_in_snap(inode)) {
> -			ceph_add_cap(inode, session,
> +			bool cap_current = ceph_add_cap(inode, session,
>  				     le64_to_cpu(info->cap.cap_id),
>  				     info_caps,
>  				     le32_to_cpu(info->cap.wanted),
> @@ -1416,6 +1875,14 @@ int ceph_fill_inode(struct inode *inode, struct folio *locked_folio,
>  				     le64_to_cpu(info->cap.realm),
>  				     info->cap.flags, &new_cap);
>  
> +			/*
> +			 * Caps above PIN mean quarantine has been lifted,
> +			 * including when the disable message was lost.
> +			 */
> +			if (cap_current && (info_caps & ~CEPH_CAP_PIN) &&
> +			    ceph_quarantine_reply_can_clear(ci, session))
> +				ceph_quarantine_clear(ci);

[Severity: High]
How can ceph_quarantine_reply_can_clear() succeed for req->r_target_inode or
later readdir entries when earlier steps in the same reply advance
mdsc->quarantine_epoch?

req->r_quarantine_epoch is sampled once in ceph_mdsc_create_request(), and
ceph_quarantine_reply_can_clear() requires req->r_quarantine_epoch == epoch
when session->s_mds != ci->i_qtine_mds. However, mdsc->quarantine_epoch is
also incremented by ceph_quarantine_clear(), ceph_quarantine_reset(), and
ceph_quarantine_splice_start()/finish():

1. In ceph_fill_trace(), ceph_fill_inode() is called on parent_dir before it
is called on req->r_target_inode:

fs/ceph/inode.c:ceph_fill_trace() {
	if (parent_dir) {
		ceph_inode_set_subvolume(parent_dir,
					 rinfo->diri.subvolume_id);
		err = ceph_fill_inode(parent_dir, NULL, &rinfo->diri,
				      rinfo->dirfrag, session, -1,
				      &req->r_caps_reservation);
		...
	}
	...
	if (rinfo->head->is_target) {
		in = req->r_target_inode;
		ceph_inode_set_subvolume(in, rinfo->targeti.subvolume_id);
		err = ceph_fill_inode(in, req->r_locked_folio, &rinfo->targeti,
				...);
	}
}

If filling parent_dir clears a quarantine mark on parent_dir,
ceph_quarantine_clear() advances mdsc->quarantine_epoch by 2, so
req->r_quarantine_epoch == epoch becomes false before ceph_fill_inode() runs
on req->r_target_inode.

2. In ceph_readdir_prepopulate(), each entry calls ceph_fill_inode() and,
for negative dentries, splice_dentry(), which calls
ceph_quarantine_splice_start() and ceph_quarantine_splice_finish(). On the
first entry that splices a dentry or clears a quarantine mark,
mdsc->quarantine_epoch advances, preventing ceph_quarantine_reply_can_clear()
from clearing quarantine on any subsequent entry in the readdir reply.

3. If a request is created before the old reporting session's cap is removed
or reset via __ceph_remove_cap(), remove_session_caps_cb(), or
reconnect_caps_cb(), ceph_quarantine_reset() advances mdsc->quarantine_epoch
after req->r_quarantine_epoch was recorded, so the reply cannot clear the
pending mark on another rank.

[ ... ]

> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index d4ef8a355a480..bb7cf9384fd6d 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2041,10 +2041,12 @@ static int remove_session_caps_cb(struct inode *inode, int mds, void *arg)
>  	struct ceph_inode_info *ci = ceph_inode(inode);
>  	struct ceph_client *cl = ceph_inode_to_client(inode);
>  	bool invalidate = false;
> +	bool reset;
>  	struct ceph_cap *cap;
>  	int iputs = 0;
>  
>  	spin_lock(&ci->i_ceph_lock);
> +	reset = ceph_quarantine_reset(ci);
>  	cap = __get_cap_for_mds(ci, mds);
>  	if (cap) {
>  		boutc(cl, " removing cap %p, ci is %p, inode is %p\n",
> @@ -2054,7 +2056,7 @@ static int remove_session_caps_cb(struct inode *inode, int mds, void *arg)
>  	}
>  	spin_unlock(&ci->i_ceph_lock);
>  
> -	if (cap)
> +	if (cap || reset)
>  		wake_up_all(&ci->i_cap_wq);
>  	if (invalidate)
>  		ceph_queue_invalidate(inode);
> @@ -2860,6 +2862,7 @@ ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode)
>  
>  	mutex_init(&req->r_fill_mutex);
>  	req->r_mdsc = mdsc;
> +	req->r_quarantine_epoch = atomic64_read_acquire(&mdsc->quarantine_epoch);
>  	req->r_started = jiffies;
>  	req->r_start_latency = ktime_get();
>  	req->r_resend_mds = -1;
> @@ -5009,6 +5012,7 @@ static int reconnect_caps_cb(struct inode *inode, int mds, void *arg)
>  	struct ceph_path_info path_info = {0};
>  	int err;
>  	u64 snap_follows;
> +	bool reset = false;
>  
>  	dentry = d_find_primary(inode);
>  	if (dentry) {
> @@ -5023,9 +5027,16 @@ static int reconnect_caps_cb(struct inode *inode, int mds, void *arg)
>  	}
>  
>  	spin_lock(&ci->i_ceph_lock);
> +	/*
> +	 * Require a fresh MDS answer after reconnect. A PIN import may
> +	 * carry no quarantine errno, so it must not reopen cached snap caps.
> +	 */
> +	reset = ceph_quarantine_reset(ci);
>  	cap = __get_cap_for_mds(ci, mds);
>  	if (!cap) {
>  		spin_unlock(&ci->i_ceph_lock);
> +		if (reset)
> +			wake_up_all(&ci->i_cap_wq);
>  		err = 0;
>  		goto out_err;
>  	}

[Severity: Medium]
Should remove_session_caps_cb() and reconnect_caps_cb() check whether
READ_ONCE(ci->i_qtine_mds) == mds before calling ceph_quarantine_reset(ci)?

In __ceph_remove_cap(), ceph_quarantine_reset(ci) is gated on
READ_ONCE(ci->i_qtine_mds) == cap->mds so that only losing the reporting
rank's cap transitions the mark to pending. However,
remove_session_caps_cb() and reconnect_caps_cb() call
ceph_quarantine_reset(ci) unconditionally for every inode visited during a
session close or reconnect, even when the quarantine mark was reported by a
different mds rank whose session is still active.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792527529.git.amarkuze@redhat.com?part=4