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

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

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.