Xiubo Li <xiubli@xxxxxxxxxx> writes: > On 6/14/22 12:07 AM, Luís Henriques wrote: >> Xiubo Li <xiubli@xxxxxxxxxx> writes: >> >>> For async create we will always try to choose the auth MDS of frag >>> the dentry belonged to of the parent directory to send the request >>> and ususally this works fine, but if the MDS migrated the directory >>> to another MDS before it could be handled the request will be >>> forwarded. And then the auth cap will be changed. >>> >>> We need to update the auth cap in this case before the request is >>> forwarded. >>> >>> URL: https://tracker.ceph.com/issues/55857 >>> Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx> >>> --- >>> fs/ceph/file.c | 12 +++++++++ >>> fs/ceph/mds_client.c | 58 ++++++++++++++++++++++++++++++++++++++++++++ >>> fs/ceph/super.h | 2 ++ >>> 3 files changed, 72 insertions(+) >>> >>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c >>> index 0e82a1c383ca..54acf76c5e9b 100644 >>> --- a/fs/ceph/file.c >>> +++ b/fs/ceph/file.c >>> @@ -613,6 +613,7 @@ static int ceph_finish_async_create(struct inode *dir, struct inode *inode, >>> struct ceph_mds_reply_inode in = { }; >>> struct ceph_mds_reply_info_in iinfo = { .in = &in }; >>> struct ceph_inode_info *ci = ceph_inode(dir); >>> + struct ceph_dentry_info *di = ceph_dentry(dentry); >>> struct timespec64 now; >>> struct ceph_string *pool_ns; >>> struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(dir->i_sb); >>> @@ -709,6 +710,12 @@ static int ceph_finish_async_create(struct inode *dir, struct inode *inode, >>> file->f_mode |= FMODE_CREATED; >>> ret = finish_open(file, dentry, ceph_open); >>> } >>> + >>> + spin_lock(&dentry->d_lock); >>> + di->flags &= ~CEPH_DENTRY_ASYNC_CREATE; >>> + wake_up_bit(&di->flags, CEPH_DENTRY_ASYNC_CREATE_BIT); >>> + spin_unlock(&dentry->d_lock); >> Question: shouldn't we initialise 'di' *after* grabbing ->d_lock? Ceph >> code doesn't seem to be consistent with this regard, but my understanding >> is that doing it this way is racy. And if so, some other places may need >> fixing. > > Yeah, it should be. > > BTW, do you mean some where like this: > > if (!test_bit(CEPH_DENTRY_ASYNC_UNLINK_BIT, &di->flags)) > > ? > > If so, it's okay and no issue here. No, I meant patterns like the one above, where you initialize a pointer to a struct ceph_dentry_info (from ->d_fsdata) and then grab ->d_lock. For example, in splice_dentry(). I think there are a few other places where this pattern can be seen, even in other filesystems. So maybe it's not really an issue, and that's why I asked: does this lock really protects accesses to ->d_fsdata? Cheers, -- Luís
