Re: [PATCH 2/3] ceph: parse subvolume_id from InodeStat v9 and store in inode

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

On Thu, 2025-11-27 at 13:46 +0000, Alex Markuze wrote:
> Add support for parsing the subvolume_id field from InodeStat v9 and
> storing it in the inode for later use by subvolume metrics tracking.
> 
> The subvolume_id identifies which CephFS subvolume an inode belongs to,
> enabling per-subvolume I/O metrics collection and reporting.
> 
> This patch:
> - Adds subvolume_id field to struct ceph_mds_reply_info_in
> - Adds i_subvolume_id field to struct ceph_inode_info
> - Parses subvolume_id from v9 InodeStat in parse_reply_info_in()
> - Adds ceph_inode_set_subvolume() helper to propagate the ID to inodes
> - Initializes i_subvolume_id in inode allocation and clears on destroy
> 
> Signed-off-by: Alex Markuze <amarkuze@redhat.com>
> ---
>  fs/ceph/inode.c      | 19 +++++++++++++++++++
>  fs/ceph/mds_client.c |  7 +++++++
>  fs/ceph/mds_client.h |  1 +
>  fs/ceph/super.h      |  2 ++
>  4 files changed, 29 insertions(+)
> 
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index a6e260d9e420..c3fb4dac4692 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -638,6 +638,7 @@ struct inode *ceph_alloc_inode(struct super_block *sb)
>  
>  	ci->i_max_bytes = 0;
>  	ci->i_max_files = 0;
> +	ci->i_subvolume_id = 0;

So, which number starts to identify subvolume ID? Is 0 valid identification
number?

If 0 is valid identification number, then I assume we need to assign some other
number because we don't know the subvolume ID yet. What's about U64_MAX, for
example, or some other constant representing the invalid value?

>  
>  	memset(&ci->i_dir_layout, 0, sizeof(ci->i_dir_layout));
>  	memset(&ci->i_cached_layout, 0, sizeof(ci->i_cached_layout));
> @@ -742,6 +743,8 @@ void ceph_evict_inode(struct inode *inode)
>  
>  	percpu_counter_dec(&mdsc->metric.total_inodes);
>  
> +	ci->i_subvolume_id = 0;

Ditto.

> +
>  	netfs_wait_for_outstanding_io(inode);
>  	truncate_inode_pages_final(&inode->i_data);
>  	if (inode->i_state & I_PINNING_NETFS_WB)
> @@ -873,6 +876,18 @@ int ceph_fill_file_size(struct inode *inode, int issued,
>  	return queue_trunc;
>  }
>  
> +void ceph_inode_set_subvolume(struct inode *inode, u64 subvolume_id)
> +{
> +	struct ceph_inode_info *ci;
> +
> +	if (!inode || !subvolume_id)

Are you sure that 0 is invalid ID for subvolume_id? What about to introduce some
named constant and to compare it with subvolume_id here? For example, we can
introduce CEPH_INVALID_SUBVOLUME_ID.

if (!inode || subvolume_id == CEPH_INVALID_SUBVOLUME_ID)

> +		return;
> +
> +	ci = ceph_inode(inode);
> +	if (READ_ONCE(ci->i_subvolume_id) != subvolume_id)
> +		WRITE_ONCE(ci->i_subvolume_id, subvolume_id);

Should the ceph_inode_set_subvolume() operation be protected by i_ceph_lock?

If ci->i_subvolume_id is already correct value, then how correct is reset of
this value? I am afraid that we could have potential bugs here. Maybe, we should
have methods of set_subvolume_id()/delete_subvolume_id()?

How looks like the interface of setting and resetting the subvolume_id?

> +}
> +
>  void ceph_fill_file_time(struct inode *inode, int issued,
>  			 u64 time_warp_seq, struct timespec64 *ctime,
>  			 struct timespec64 *mtime, struct timespec64 *atime)
> @@ -1087,6 +1102,7 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page,
>  	new_issued = ~issued & info_caps;
>  
>  	__ceph_update_quota(ci, iinfo->max_bytes, iinfo->max_files);
> +	ceph_inode_set_subvolume(inode, iinfo->subvolume_id);
>  
>  #ifdef CONFIG_FS_ENCRYPTION
>  	if (iinfo->fscrypt_auth_len &&
> @@ -1594,6 +1610,8 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
>  			goto done;
>  		}
>  		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);
> @@ -1682,6 +1700,7 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
>  		BUG_ON(!req->r_target_inode);
>  
>  		in = req->r_target_inode;
> +		ceph_inode_set_subvolume(in, rinfo->targeti.subvolume_id);
>  		err = ceph_fill_inode(in, req->r_locked_page, &rinfo->targeti,
>  				NULL, session,
>  				(!test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags) &&
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 32561fc701e5..6f66097f740b 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -105,6 +105,8 @@ static int parse_reply_info_in(void **p, void *end,
>  	int err = 0;
>  	u8 struct_v = 0;
>  
> +	info->subvolume_id = 0;

Should we have method for struct ceph_mds_reply_info_in likewise for struct
ceph_inode_info?

> +
>  	if (features == (u64)-1) {
>  		u32 struct_len;
>  		u8 struct_compat;
> @@ -243,6 +245,10 @@ static int parse_reply_info_in(void **p, void *end,
>  			ceph_decode_skip_n(p, end, v8_struct_len, bad);
>  		}
>  
> +		/* struct_v 9 added subvolume_id */
> +		if (struct_v >= 9)
> +			ceph_decode_64_safe(p, end, info->subvolume_id, bad);

As far as I remember from previous patch we had version 8 and we don't support
it. Do we mean the version 9 of another protocol here?

Thanks,
Slava.

> +
>  		*p = end;
>  	} else {
>  		/* legacy (unversioned) struct */
> @@ -3962,6 +3968,7 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
>  			goto out_err;
>  		}
>  		req->r_target_inode = in;
> +		ceph_inode_set_subvolume(in, rinfo->targeti.subvolume_id);
>  	}
>  
>  	mutex_lock(&session->s_mutex);
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index 0428a5eaf28c..bd3690baa65c 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -118,6 +118,7 @@ struct ceph_mds_reply_info_in {
>  	u32 fscrypt_file_len;
>  	u64 rsnaps;
>  	u64 change_attr;
> +	u64 subvolume_id;
>  };
>  
>  struct ceph_mds_reply_dir_entry {
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index a1f781c46b41..69069c920683 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -385,6 +385,7 @@ struct ceph_inode_info {
>  
>  	/* quotas */
>  	u64 i_max_bytes, i_max_files;
> +	u64 i_subvolume_id;
>  
>  	s32 i_dir_pin;
>  
> @@ -1057,6 +1058,7 @@ extern struct inode *ceph_get_inode(struct super_block *sb,
>  extern struct inode *ceph_get_snapdir(struct inode *parent);
>  extern int ceph_fill_file_size(struct inode *inode, int issued,
>  			       u32 truncate_seq, u64 truncate_size, u64 size);
> +extern void ceph_inode_set_subvolume(struct inode *inode, u64 subvolume_id);
>  extern void ceph_fill_file_time(struct inode *inode, int issued,
>  				u64 time_warp_seq, struct timespec64 *ctime,
>  				struct timespec64 *mtime,