On Wed, Sep 13, 2023 at 08:09:59AM -0300, Christoph Hellwig wrote: > diff --git a/fs/super.c b/fs/super.c > index bbe55f0651cca4..5c685b4944c2d6 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -787,7 +787,7 @@ struct super_block *sget_fc(struct fs_context *fc, > struct super_block *s = NULL; > struct super_block *old; > struct user_namespace *user_ns = fc->global ? &init_user_ns : fc->user_ns; > - int err; > + int err = 0; > > retry: > spin_lock(&sb_lock); > @@ -806,14 +806,26 @@ struct super_block *sget_fc(struct fs_context *fc, > } > > s->s_fs_info = fc->s_fs_info; > - err = set(s, fc); > - if (err) { > - s->s_fs_info = NULL; > - spin_unlock(&sb_lock); > - destroy_unused_super(s); > - return ERR_PTR(err); > + if (set) { > + err = set(s, fc); > + if (err) { > + s->s_fs_info = NULL; Pointless (as the original had been); destroy_unused_super() doesn't even look at ->s_fs_info. > + goto unlock_and_destroy; > + } > } > fc->s_fs_info = NULL; Here we are transferring the ownership from fc to superblock; it used to sit right next to insertion into lists and all failure exits past that point must go through deactivate_locked_super(), so ->kill_sb() would be called on those and it would take care of s->s_fs_info. However, your variant has that ownership transfer done at the point before get_anon_bdev(), and that got you a new failure exit where you are still calling destroy_unused_super(): > + if (!s->s_dev) { > + /* > + * If the file system didn't set a s_dev (which is usually only > + * done for block based file systems), set an anonymous dev_t > + * here now so that we always have a valid ->s_dev. > + */ > + err = get_anon_bdev(&s->s_dev); > + if (err) > + goto unlock_and_destroy; This. And that's a leak - fc has no reference left in it, and your unlock_and_destroy won't even look at what's in ->s_fs_info, let alone know what to do with it. IOW, clearing fc->s_fs_info should've been done after that chunk. And looking at the change in sget(), > + if (set) { > + err = set(s, data); > + if (err) > + goto unlock_and_destroy; > } > + > + if (!s->s_dev) { > + err = get_anon_bdev(&s->s_dev); > + if (err) > + goto unlock_and_destroy; > + } I'd rather expressed it (both there and in sget_fc() as well) as if (set) err = set(s, data); if (!err && !s->s_dev) err = get_anon_bdev(&s->s_dev); if (err) goto unlock_and_destroy; That's really what your transformation does - you are lifting the calls of get_anon_bdev() (in guise of set_anon_super()) from the tails of 'set' callbacks into the caller, making them conditional upon the lack of other errors from 'set' and upon the ->s_dev left zero and allow NULL for the case when that was all that had been there. The only place where you do something different is this: > @@ -1191,7 +1191,6 @@ static struct dentry *ceph_real_mount(struct ceph_fs_client *fsc, > static int ceph_set_super(struct super_block *s, struct fs_context *fc) > { > struct ceph_fs_client *fsc = s->s_fs_info; > - int ret; > > dout("set_super %p\n", s); > > @@ -1211,11 +1210,7 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc) > s->s_flags |= SB_NODIRATIME | SB_NOATIME; > > ceph_fscrypt_set_ops(s); > - > - ret = set_anon_super_fc(s, fc); > - if (ret != 0) > - fsc->sb = NULL; > - return ret; > + return 0; fsc->sb = NULL has disappeared here; it *is* OK (the caller won't look at fsc->sb after failed sget_fc()), but that's worth a mention somewhere. A separate commit removing that clearing fsc->sb in ceph_set_super(), perhaps?