[PATCH] ceph: fix missing snapshot context in write operations

[PATCH] ceph: fix missing snapshot context in write operations

[ethanwu]

This series addresses two instances where Ceph filesystem operations
were missing proper snapshot context handling, which could lead to
data inconsistencies in snapshots.

The issue occurs in two scenarios:
1. ceph_zero_partial_object() during fallocate punch hole operations
2. ceph_uninline_data() when converting inline data to regular objects

Both functions were passing NULL snapshot context to OSD write operations
instead of acquiring the appropriate context from either pending cap snaps
or the inode's head snapc. This could result in snapshot data corruption
where subsequent reads from snapshots would return modified data instead
of the original snapshot content.

The fix ensures that proper snapshot context is acquired and passed to
all OSD write operations in these code paths.

ethanwu (2):
  ceph: fix snapshot context missing in ceph_zero_partial_object
  ceph: fix snapshot context missing in ceph_uninline_data

 fs/ceph/addr.c | 19 +++++++++++++++++--
 fs/ceph/file.c | 17 ++++++++++++++++-
 2 files changed, 33 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH] ceph: fix snapshot context missing in ceph_zero_partial_object

[ethanwu]

The ceph_zero_partial_object function was missing proper snapshot
context for its OSD write operations, which could lead to data
inconsistencies in snapshots.

Reproducer:
dd if=/dev/urandom of=/mnt/mycephfs/foo bs=64K count=1
mkdir /mnt/mycephfs/.snap/snap1
md5sum /mnt/mycephfs/.snap/snap1/foo
fallocate -p -o 0 -l 4096 /mnt/mycephfs/foo
echo 3 > /proc/sys/vm/drop/caches
md5sum /mnt/mycephfs/.snap/snap1/foo # get different md5sum!!

will get the same

Fixes: ad7a60de882ac ("ceph: punch hole support")
Signed-off-by: ethanwu <ethanwu@synology.com>
---
 fs/ceph/file.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index c02f100f8552..58cc2cbae8bc 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -2572,6 +2572,7 @@ static int ceph_zero_partial_object(struct inode *inode,
 	struct ceph_inode_info *ci = ceph_inode(inode);
 	struct ceph_fs_client *fsc = ceph_inode_to_fs_client(inode);
 	struct ceph_osd_request *req;
+	struct ceph_snap_context *snapc;
 	int ret = 0;
 	loff_t zero = 0;
 	int op;
@@ -2586,12 +2587,25 @@ static int ceph_zero_partial_object(struct inode *inode,
 		op = CEPH_OSD_OP_ZERO;
 	}
 
+	spin_lock(&ci->i_ceph_lock);
+	if (__ceph_have_pending_cap_snap(ci)) {
+		struct ceph_cap_snap *capsnap =
+				list_last_entry(&ci->i_cap_snaps,
+						struct ceph_cap_snap,
+						ci_item);
+		snapc = ceph_get_snap_context(capsnap->context);
+	} else {
+		BUG_ON(!ci->i_head_snapc);
+		snapc = ceph_get_snap_context(ci->i_head_snapc);
+	}
+	spin_unlock(&ci->i_ceph_lock);
+
 	req = ceph_osdc_new_request(&fsc->client->osdc, &ci->i_layout,
 					ceph_vino(inode),
 					offset, length,
 					0, 1, op,
 					CEPH_OSD_FLAG_WRITE,
-					NULL, 0, 0, false);
+					snapc, 0, 0, false);
 	if (IS_ERR(req)) {
 		ret = PTR_ERR(req);
 		goto out;
@@ -2605,6 +2619,7 @@ static int ceph_zero_partial_object(struct inode *inode,
 	ceph_osdc_put_request(req);
 
 out:
+	ceph_put_snap_context(snapc);
 	return ret;
 }
 
-- 
2.43.0

[PATCH] ceph: fix snapshot context missing in ceph_uninline_data

[ethanwu]

The ceph_uninline_data function was missing proper snapshot context
handling for its OSD write operations. Both CEPH_OSD_OP_CREATE and
CEPH_OSD_OP_WRITE requests were passing NULL instead of the appropriate
snapshot context, which could lead to data inconsistencies in snapshots.

This fix properly acquiring the snapshot context from either pending
cap snaps or the inode's head snapc before performing write operations.

Signed-off-by: ethanwu <ethanwu@synology.com>
---
 fs/ceph/addr.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 8b202d789e93..a8aeca9654b6 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -2202,6 +2202,7 @@ int ceph_uninline_data(struct file *file)
 	struct ceph_osd_request *req = NULL;
 	struct ceph_cap_flush *prealloc_cf = NULL;
 	struct folio *folio = NULL;
+	struct ceph_snap_context *snapc = NULL;
 	u64 inline_version = CEPH_INLINE_NONE;
 	struct page *pages[1];
 	int err = 0;
@@ -2229,6 +2230,19 @@ int ceph_uninline_data(struct file *file)
 	if (inline_version == 1) /* initial version, no data */
 		goto out_uninline;
 
+	spin_lock(&ci->i_ceph_lock);
+	if (__ceph_have_pending_cap_snap(ci)) {
+		struct ceph_cap_snap *capsnap =
+				list_last_entry(&ci->i_cap_snaps,
+						struct ceph_cap_snap,
+						ci_item);
+		snapc = ceph_get_snap_context(capsnap->context);
+	} else {
+		BUG_ON(!ci->i_head_snapc);
+		snapc = ceph_get_snap_context(ci->i_head_snapc);
+	}
+	spin_unlock(&ci->i_ceph_lock);
+
 	folio = read_mapping_folio(inode->i_mapping, 0, file);
 	if (IS_ERR(folio)) {
 		err = PTR_ERR(folio);
@@ -2244,7 +2258,7 @@ int ceph_uninline_data(struct file *file)
 	req = ceph_osdc_new_request(&fsc->client->osdc, &ci->i_layout,
 				    ceph_vino(inode), 0, &len, 0, 1,
 				    CEPH_OSD_OP_CREATE, CEPH_OSD_FLAG_WRITE,
-				    NULL, 0, 0, false);
+				    snapc, 0, 0, false);
 	if (IS_ERR(req)) {
 		err = PTR_ERR(req);
 		goto out_unlock;
@@ -2260,7 +2274,7 @@ int ceph_uninline_data(struct file *file)
 	req = ceph_osdc_new_request(&fsc->client->osdc, &ci->i_layout,
 				    ceph_vino(inode), 0, &len, 1, 3,
 				    CEPH_OSD_OP_WRITE, CEPH_OSD_FLAG_WRITE,
-				    NULL, ci->i_truncate_seq,
+				    snapc, ci->i_truncate_seq,
 				    ci->i_truncate_size, false);
 	if (IS_ERR(req)) {
 		err = PTR_ERR(req);
@@ -2323,6 +2337,7 @@ int ceph_uninline_data(struct file *file)
 		folio_put(folio);
 	}
 out:
+	ceph_put_snap_context(snapc);
 	ceph_free_cap_flush(prealloc_cf);
 	doutc(cl, "%llx.%llx inline_version %llu = %d\n",
 	      ceph_vinop(inode), inline_version, err);
-- 
2.43.0

Re: [PATCH] ceph: fix missing snapshot context in write operations

[Viacheslav Dubeyko]

On Wed, 2025-09-24 at 17:58 +0800, ethanwu wrote:
> This series addresses two instances where Ceph filesystem operations
> were missing proper snapshot context handling, which could lead to
> data inconsistencies in snapshots.
> 

It doesn't look like series. You can prepare patch set by using command:

git format-patch --cover-letter -2 HEAD

Thanks,
Slava.

> The issue occurs in two scenarios:
> 1. ceph_zero_partial_object() during fallocate punch hole operations
> 2. ceph_uninline_data() when converting inline data to regular objects
> 
> Both functions were passing NULL snapshot context to OSD write operations
> instead of acquiring the appropriate context from either pending cap snaps
> or the inode's head snapc. This could result in snapshot data corruption
> where subsequent reads from snapshots would return modified data instead
> of the original snapshot content.
> 
> The fix ensures that proper snapshot context is acquired and passed to
> all OSD write operations in these code paths.
> 
> ethanwu (2):
>   ceph: fix snapshot context missing in ceph_zero_partial_object
>   ceph: fix snapshot context missing in ceph_uninline_data
> 
>  fs/ceph/addr.c | 19 +++++++++++++++++--
>  fs/ceph/file.c | 17 ++++++++++++++++-
>  2 files changed, 33 insertions(+), 3 deletions(-)

Re: [PATCH] ceph: fix snapshot context missing in ceph_zero_partial_object

[Viacheslav Dubeyko]

On Wed, 2025-09-24 at 17:58 +0800, ethanwu wrote:
> The ceph_zero_partial_object function was missing proper snapshot
> context for its OSD write operations, which could lead to data
> inconsistencies in snapshots.
> 
> Reproducer:
> dd if=/dev/urandom of=/mnt/mycephfs/foo bs=64K count=1
> mkdir /mnt/mycephfs/.snap/snap1
> md5sum /mnt/mycephfs/.snap/snap1/foo
> fallocate -p -o 0 -l 4096 /mnt/mycephfs/foo
> echo 3 > /proc/sys/vm/drop/caches
> md5sum /mnt/mycephfs/.snap/snap1/foo # get different md5sum!!
> 

I assume that it's not complete reproduction recipe. It needs to enable the
support of snapshot feature as well.

> will get the same
> 
> Fixes: ad7a60de882ac ("ceph: punch hole support")
> Signed-off-by: ethanwu <ethanwu@synology.com>
> ---
>  fs/ceph/file.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index c02f100f8552..58cc2cbae8bc 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -2572,6 +2572,7 @@ static int ceph_zero_partial_object(struct inode *inode,
>  	struct ceph_inode_info *ci = ceph_inode(inode);
>  	struct ceph_fs_client *fsc = ceph_inode_to_fs_client(inode);
>  	struct ceph_osd_request *req;
> +	struct ceph_snap_context *snapc;
>  	int ret = 0;
>  	loff_t zero = 0;
>  	int op;
> @@ -2586,12 +2587,25 @@ static int ceph_zero_partial_object(struct inode *inode,

As far as I can see, you are covering the zeroing case. I assume that other type
of operations (like modifying the file's content) works well. Am I correct? Have
you tested this?

We definitely have not enough xfstests and unit-tests. We haven't Ceph specific
test in xfstests for covering snapshots functionality. It will be really great
if somebody can write this test(s). :)

Thanks,
Slava.

>  		op = CEPH_OSD_OP_ZERO;
>  	}
>  
> +	spin_lock(&ci->i_ceph_lock);
> +	if (__ceph_have_pending_cap_snap(ci)) {
> +		struct ceph_cap_snap *capsnap =
> +				list_last_entry(&ci->i_cap_snaps,
> +						struct ceph_cap_snap,
> +						ci_item);
> +		snapc = ceph_get_snap_context(capsnap->context);
> +	} else {
> +		BUG_ON(!ci->i_head_snapc);
> +		snapc = ceph_get_snap_context(ci->i_head_snapc);
> +	}
> +	spin_unlock(&ci->i_ceph_lock);
> +
>  	req = ceph_osdc_new_request(&fsc->client->osdc, &ci->i_layout,
>  					ceph_vino(inode),
>  					offset, length,
>  					0, 1, op,
>  					CEPH_OSD_FLAG_WRITE,
> -					NULL, 0, 0, false);
> +					snapc, 0, 0, false);
>  	if (IS_ERR(req)) {
>  		ret = PTR_ERR(req);
>  		goto out;
> @@ -2605,6 +2619,7 @@ static int ceph_zero_partial_object(struct inode *inode,
>  	ceph_osdc_put_request(req);
>  
>  out:
> +	ceph_put_snap_context(snapc);
>  	return ret;
>  }
>  

Re: [PATCH] ceph: fix snapshot context missing in ceph_uninline_data

[Viacheslav Dubeyko]

On Wed, 2025-09-24 at 17:58 +0800, ethanwu wrote:
> The ceph_uninline_data function was missing proper snapshot context
> handling for its OSD write operations. Both CEPH_OSD_OP_CREATE and
> CEPH_OSD_OP_WRITE requests were passing NULL instead of the appropriate
> snapshot context, which could lead to data inconsistencies in snapshots.
> 

What is the reproducing path for this issue? How is it possible to check that
something is going wrong or functionality works right?

> This fix properly acquiring the snapshot context from either pending
> cap snaps or the inode's head snapc before performing write operations.
> 

Have you split the whole fix on two patches because of different use-cases?

Thanks,
Slava.

> Signed-off-by: ethanwu <ethanwu@synology.com>
> ---
>  fs/ceph/addr.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 8b202d789e93..a8aeca9654b6 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -2202,6 +2202,7 @@ int ceph_uninline_data(struct file *file)
>  	struct ceph_osd_request *req = NULL;
>  	struct ceph_cap_flush *prealloc_cf = NULL;
>  	struct folio *folio = NULL;
> +	struct ceph_snap_context *snapc = NULL;
>  	u64 inline_version = CEPH_INLINE_NONE;
>  	struct page *pages[1];
>  	int err = 0;
> @@ -2229,6 +2230,19 @@ int ceph_uninline_data(struct file *file)
>  	if (inline_version == 1) /* initial version, no data */
>  		goto out_uninline;
>  
> +	spin_lock(&ci->i_ceph_lock);
> +	if (__ceph_have_pending_cap_snap(ci)) {
> +		struct ceph_cap_snap *capsnap =
> +				list_last_entry(&ci->i_cap_snaps,
> +						struct ceph_cap_snap,
> +						ci_item);
> +		snapc = ceph_get_snap_context(capsnap->context);
> +	} else {
> +		BUG_ON(!ci->i_head_snapc);
> +		snapc = ceph_get_snap_context(ci->i_head_snapc);
> +	}
> +	spin_unlock(&ci->i_ceph_lock);
> +
>  	folio = read_mapping_folio(inode->i_mapping, 0, file);
>  	if (IS_ERR(folio)) {
>  		err = PTR_ERR(folio);
> @@ -2244,7 +2258,7 @@ int ceph_uninline_data(struct file *file)
>  	req = ceph_osdc_new_request(&fsc->client->osdc, &ci->i_layout,
>  				    ceph_vino(inode), 0, &len, 0, 1,
>  				    CEPH_OSD_OP_CREATE, CEPH_OSD_FLAG_WRITE,
> -				    NULL, 0, 0, false);
> +				    snapc, 0, 0, false);
>  	if (IS_ERR(req)) {
>  		err = PTR_ERR(req);
>  		goto out_unlock;
> @@ -2260,7 +2274,7 @@ int ceph_uninline_data(struct file *file)
>  	req = ceph_osdc_new_request(&fsc->client->osdc, &ci->i_layout,
>  				    ceph_vino(inode), 0, &len, 1, 3,
>  				    CEPH_OSD_OP_WRITE, CEPH_OSD_FLAG_WRITE,
> -				    NULL, ci->i_truncate_seq,
> +				    snapc, ci->i_truncate_seq,
>  				    ci->i_truncate_size, false);
>  	if (IS_ERR(req)) {
>  		err = PTR_ERR(req);
> @@ -2323,6 +2337,7 @@ int ceph_uninline_data(struct file *file)
>  		folio_put(folio);
>  	}
>  out:
> +	ceph_put_snap_context(snapc);
>  	ceph_free_cap_flush(prealloc_cf);
>  	doutc(cl, "%llx.%llx inline_version %llu = %d\n",
>  	      ceph_vinop(inode), inline_version, err);

Re: [PATCH] ceph: fix snapshot context missing in ceph_zero_partial_object

[tzuchieh wu]

Viacheslav Dubeyko <Slava.Dubeyko@ibm.com> 於 2025年9月25日 週四 上午2:26寫道：
>
> On Wed, 2025-09-24 at 17:58 +0800, ethanwu wrote:
> > The ceph_zero_partial_object function was missing proper snapshot
> > context for its OSD write operations, which could lead to data
> > inconsistencies in snapshots.
> >
> > Reproducer:
> > dd if=/dev/urandom of=/mnt/mycephfs/foo bs=64K count=1
> > mkdir /mnt/mycephfs/.snap/snap1
> > md5sum /mnt/mycephfs/.snap/snap1/foo
> > fallocate -p -o 0 -l 4096 /mnt/mycephfs/foo
> > echo 3 > /proc/sys/vm/drop/caches
> > md5sum /mnt/mycephfs/.snap/snap1/foo # get different md5sum!!
> >
>
> I assume that it's not complete reproduction recipe. It needs to enable the
> support of snapshot feature as well.

I use ../src/vstart.sh --new -x --localhost --bluestore
to deploy the cephfs environment and the fs allow_snaps is enabled by default.
but client snap auth is needed.
I use the following command to grant the auth
./bin/ceph auth caps client.fs_a mds 'allow rwps fsname=a' mon 'allow
r fsname=a' osd 'allow rw tag cephfs data=a'

>
> > will get the same
> >
> > Fixes: ad7a60de882ac ("ceph: punch hole support")
> > Signed-off-by: ethanwu <ethanwu@synology.com>
> > ---
> >  fs/ceph/file.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > index c02f100f8552..58cc2cbae8bc 100644
> > --- a/fs/ceph/file.c
> > +++ b/fs/ceph/file.c
> > @@ -2572,6 +2572,7 @@ static int ceph_zero_partial_object(struct inode *inode,
> >       struct ceph_inode_info *ci = ceph_inode(inode);
> >       struct ceph_fs_client *fsc = ceph_inode_to_fs_client(inode);
> >       struct ceph_osd_request *req;
> > +     struct ceph_snap_context *snapc;
> >       int ret = 0;
> >       loff_t zero = 0;
> >       int op;
> > @@ -2586,12 +2587,25 @@ static int ceph_zero_partial_object(struct inode *inode,
>
> As far as I can see, you are covering the zeroing case. I assume that other type
> of operations (like modifying the file's content) works well. Am I correct? Have
> you tested this?

Yes, I've checked all ceph_osdc_new_request
Only these 2 places misses snap context, write operation works as expected.

>
> We definitely have not enough xfstests and unit-tests. We haven't Ceph specific
> test in xfstests for covering snapshots functionality. It will be really great
> if somebody can write this test(s). :)
>

I can add this test into xfstests or ceph unit-tests,
which place do you prefer to add this test on?

> Thanks,
> Slava.
>

Thanks,
ethanwu

RE: [PATCH] ceph: fix snapshot context missing in ceph_zero_partial_object

[Viacheslav Dubeyko]

On Thu, 2025-09-25 at 18:24 +0800, tzuchieh wu wrote:
> Viacheslav Dubeyko <Slava.Dubeyko@ibm.com> 於 2025年9月25日 週四 上午2:26寫道：
> > 
> > On Wed, 2025-09-24 at 17:58 +0800, ethanwu wrote:
> > > The ceph_zero_partial_object function was missing proper snapshot
> > > context for its OSD write operations, which could lead to data
> > > inconsistencies in snapshots.
> > > 
> > > Reproducer:
> > > dd if=/dev/urandom of=/mnt/mycephfs/foo bs=64K count=1
> > > mkdir /mnt/mycephfs/.snap/snap1
> > > md5sum /mnt/mycephfs/.snap/snap1/foo
> > > fallocate -p -o 0 -l 4096 /mnt/mycephfs/foo
> > > echo 3 > /proc/sys/vm/drop/caches
> > > md5sum /mnt/mycephfs/.snap/snap1/foo # get different md5sum!!
> > > 
> > 
> > I assume that it's not complete reproduction recipe. It needs to enable the
> > support of snapshot feature as well.
> 
> I use ../src/vstart.sh --new -x --localhost --bluestore
> to deploy the cephfs environment and the fs allow_snaps is enabled by default.
> but client snap auth is needed.
> I use the following command to grant the auth
> ./bin/ceph auth caps client.fs_a mds 'allow rwps fsname=a' mon 'allow
> r fsname=a' osd 'allow rw tag cephfs data=a'
> 
> > 
> > > will get the same
> > > 
> > > Fixes: ad7a60de882ac ("ceph: punch hole support")
> > > Signed-off-by: ethanwu <ethanwu@synology.com>
> > > ---
> > >  fs/ceph/file.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > > index c02f100f8552..58cc2cbae8bc 100644
> > > --- a/fs/ceph/file.c
> > > +++ b/fs/ceph/file.c
> > > @@ -2572,6 +2572,7 @@ static int ceph_zero_partial_object(struct inode *inode,
> > >       struct ceph_inode_info *ci = ceph_inode(inode);
> > >       struct ceph_fs_client *fsc = ceph_inode_to_fs_client(inode);
> > >       struct ceph_osd_request *req;
> > > +     struct ceph_snap_context *snapc;
> > >       int ret = 0;
> > >       loff_t zero = 0;
> > >       int op;
> > > @@ -2586,12 +2587,25 @@ static int ceph_zero_partial_object(struct inode *inode,
> > 
> > As far as I can see, you are covering the zeroing case. I assume that other type
> > of operations (like modifying the file's content) works well. Am I correct? Have
> > you tested this?
> 
> Yes, I've checked all ceph_osdc_new_request
> Only these 2 places misses snap context, write operation works as expected.
> 

Great! Makes sense.

> > 
> > We definitely have not enough xfstests and unit-tests. We haven't Ceph specific
> > test in xfstests for covering snapshots functionality. It will be really great
> > if somebody can write this test(s). :)
> > 
> 
> I can add this test into xfstests or ceph unit-tests,
> which place do you prefer to add this test on?
> 
> > 

We have already several Ceph specialized tests in xfstests suite. So, it will be
slightly more easy to add the another test there. I have started to work on
unit-tests for CephFS kernel client recently. We don't have the Kunit-based
unit-test infrastructure for Ceph in upstream yet. I think xfstests could be
more useful and easy target right now.

Thanks,
Slava.