| Message ID | 20241216142156.4133267-2-yangerkun@huaweicloud.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | nfsd/sunrpc: cleanup resource with sync mode | expand |
On 12/16/24 9:21 AM, Yang Erkun wrote: > From: Yang Erkun <yangerkun@huawei.com> > > This reverts commit f8c989a0c89a75d30f899a7cabdc14d72522bb8d. The reverted commit is in v6.13-rc1, looks like. Should I apply 1/5 to v6.13-rc ? The remaining patches in this series will need review and testing, so I would like to reserve them for a later merge window, if that is OK with you. > Before this commit, svc_export_put or expkey_put will call path_put with > sync mode. After this commit, path_put will be called with async mode. > And this can lead the unexpected results show as follow. > > mkfs.xfs -f /dev/sda > echo "/ *(rw,no_root_squash,fsid=0)" > /etc/exports > echo "/mnt *(rw,no_root_squash,fsid=1)" >> /etc/exports > exportfs -ra > service nfs-server start > mount -t nfs -o vers=4.0 127.0.0.1:/mnt /mnt1 > mount /dev/sda /mnt/sda > touch /mnt1/sda/file > exportfs -r > umount /mnt/sda # failed unexcepted > > The touch will finally call nfsd_cross_mnt, add refcount to mount, and > then add cache_head. Before this commit, exportfs -r will call > cache_flush to cleanup all cache_head, and path_put in > svc_export_put/expkey_put will be finished with sync mode. So, the > latter umount will always success. However, after this commit, path_put > will be called with async mode, the latter umount may failed, and if > we add some delay, umount will success too. Personally I think this bug > and should be fixed. We first revert before bugfix patch, and then fix > the original bug with a different way. > > Fixes: f8c989a0c89a ("nfsd: release svc_expkey/svc_export with rcu_work") > Signed-off-by: Yang Erkun <yangerkun@huawei.com> > --- > fs/nfsd/export.c | 31 ++++++------------------------- > fs/nfsd/export.h | 4 ++-- > 2 files changed, 8 insertions(+), 27 deletions(-) > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c > index eacafe46e3b6..aa4712362b3b 100644 > --- a/fs/nfsd/export.c > +++ b/fs/nfsd/export.c > @@ -40,24 +40,15 @@ > #define EXPKEY_HASHMAX (1 << EXPKEY_HASHBITS) > #define EXPKEY_HASHMASK (EXPKEY_HASHMAX -1) > > -static void expkey_put_work(struct work_struct *work) > +static void expkey_put(struct kref *ref) > { > - struct svc_expkey *key = > - container_of(to_rcu_work(work), struct svc_expkey, ek_rcu_work); > + struct svc_expkey *key = container_of(ref, struct svc_expkey, h.ref); > > if (test_bit(CACHE_VALID, &key->h.flags) && > !test_bit(CACHE_NEGATIVE, &key->h.flags)) > path_put(&key->ek_path); > auth_domain_put(key->ek_client); > - kfree(key); > -} > - > -static void expkey_put(struct kref *ref) > -{ > - struct svc_expkey *key = container_of(ref, struct svc_expkey, h.ref); > - > - INIT_RCU_WORK(&key->ek_rcu_work, expkey_put_work); > - queue_rcu_work(system_wq, &key->ek_rcu_work); > + kfree_rcu(key, ek_rcu); > } > > static int expkey_upcall(struct cache_detail *cd, struct cache_head *h) > @@ -364,26 +355,16 @@ static void export_stats_destroy(struct export_stats *stats) > EXP_STATS_COUNTERS_NUM); > } > > -static void svc_export_put_work(struct work_struct *work) > +static void svc_export_put(struct kref *ref) > { > - struct svc_export *exp = > - container_of(to_rcu_work(work), struct svc_export, ex_rcu_work); > - > + struct svc_export *exp = container_of(ref, struct svc_export, h.ref); > path_put(&exp->ex_path); > auth_domain_put(exp->ex_client); > nfsd4_fslocs_free(&exp->ex_fslocs); > export_stats_destroy(exp->ex_stats); > kfree(exp->ex_stats); > kfree(exp->ex_uuid); > - kfree(exp); > -} > - > -static void svc_export_put(struct kref *ref) > -{ > - struct svc_export *exp = container_of(ref, struct svc_export, h.ref); > - > - INIT_RCU_WORK(&exp->ex_rcu_work, svc_export_put_work); > - queue_rcu_work(system_wq, &exp->ex_rcu_work); > + kfree_rcu(exp, ex_rcu); > } > > static int svc_export_upcall(struct cache_detail *cd, struct cache_head *h) > diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h > index 6f2fbaae01fa..4d92b99c1ffd 100644 > --- a/fs/nfsd/export.h > +++ b/fs/nfsd/export.h > @@ -75,7 +75,7 @@ struct svc_export { > u32 ex_layout_types; > struct nfsd4_deviceid_map *ex_devid_map; > struct cache_detail *cd; > - struct rcu_work ex_rcu_work; > + struct rcu_head ex_rcu; > unsigned long ex_xprtsec_modes; > struct export_stats *ex_stats; > }; > @@ -92,7 +92,7 @@ struct svc_expkey { > u32 ek_fsid[6]; > > struct path ek_path; > - struct rcu_work ek_rcu_work; > + struct rcu_head ek_rcu; > }; > > #define EX_ISSYNC(exp) (!((exp)->ex_flags & NFSEXP_ASYNC))
在 2024/12/16 22:52, Chuck Lever 写道: > On 12/16/24 9:21 AM, Yang Erkun wrote: >> From: Yang Erkun <yangerkun@huawei.com> >> >> This reverts commit f8c989a0c89a75d30f899a7cabdc14d72522bb8d. > > The reverted commit is in v6.13-rc1, looks like. Should I apply 1/5 to > v6.13-rc ? > > The remaining patches in this series will need review and testing, > so I would like to reserve them for a later merge window, if that > is OK with you. Thanks, it's ok to me! > > >> Before this commit, svc_export_put or expkey_put will call path_put with >> sync mode. After this commit, path_put will be called with async mode. >> And this can lead the unexpected results show as follow. >> >> mkfs.xfs -f /dev/sda >> echo "/ *(rw,no_root_squash,fsid=0)" > /etc/exports >> echo "/mnt *(rw,no_root_squash,fsid=1)" >> /etc/exports >> exportfs -ra >> service nfs-server start >> mount -t nfs -o vers=4.0 127.0.0.1:/mnt /mnt1 >> mount /dev/sda /mnt/sda >> touch /mnt1/sda/file >> exportfs -r >> umount /mnt/sda # failed unexcepted >> >> The touch will finally call nfsd_cross_mnt, add refcount to mount, and >> then add cache_head. Before this commit, exportfs -r will call >> cache_flush to cleanup all cache_head, and path_put in >> svc_export_put/expkey_put will be finished with sync mode. So, the >> latter umount will always success. However, after this commit, path_put >> will be called with async mode, the latter umount may failed, and if >> we add some delay, umount will success too. Personally I think this bug >> and should be fixed. We first revert before bugfix patch, and then fix >> the original bug with a different way. >> >> Fixes: f8c989a0c89a ("nfsd: release svc_expkey/svc_export with rcu_work") >> Signed-off-by: Yang Erkun <yangerkun@huawei.com> >> --- >> fs/nfsd/export.c | 31 ++++++------------------------- >> fs/nfsd/export.h | 4 ++-- >> 2 files changed, 8 insertions(+), 27 deletions(-) >> >> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c >> index eacafe46e3b6..aa4712362b3b 100644 >> --- a/fs/nfsd/export.c >> +++ b/fs/nfsd/export.c >> @@ -40,24 +40,15 @@ >> #define EXPKEY_HASHMAX (1 << EXPKEY_HASHBITS) >> #define EXPKEY_HASHMASK (EXPKEY_HASHMAX -1) >> -static void expkey_put_work(struct work_struct *work) >> +static void expkey_put(struct kref *ref) >> { >> - struct svc_expkey *key = >> - container_of(to_rcu_work(work), struct svc_expkey, ek_rcu_work); >> + struct svc_expkey *key = container_of(ref, struct svc_expkey, >> h.ref); >> if (test_bit(CACHE_VALID, &key->h.flags) && >> !test_bit(CACHE_NEGATIVE, &key->h.flags)) >> path_put(&key->ek_path); >> auth_domain_put(key->ek_client); >> - kfree(key); >> -} >> - >> -static void expkey_put(struct kref *ref) >> -{ >> - struct svc_expkey *key = container_of(ref, struct svc_expkey, >> h.ref); >> - >> - INIT_RCU_WORK(&key->ek_rcu_work, expkey_put_work); >> - queue_rcu_work(system_wq, &key->ek_rcu_work); >> + kfree_rcu(key, ek_rcu); >> } >> static int expkey_upcall(struct cache_detail *cd, struct cache_head *h) >> @@ -364,26 +355,16 @@ static void export_stats_destroy(struct >> export_stats *stats) >> EXP_STATS_COUNTERS_NUM); >> } >> -static void svc_export_put_work(struct work_struct *work) >> +static void svc_export_put(struct kref *ref) >> { >> - struct svc_export *exp = >> - container_of(to_rcu_work(work), struct svc_export, ex_rcu_work); >> - >> + struct svc_export *exp = container_of(ref, struct svc_export, >> h.ref); >> path_put(&exp->ex_path); >> auth_domain_put(exp->ex_client); >> nfsd4_fslocs_free(&exp->ex_fslocs); >> export_stats_destroy(exp->ex_stats); >> kfree(exp->ex_stats); >> kfree(exp->ex_uuid); >> - kfree(exp); >> -} >> - >> -static void svc_export_put(struct kref *ref) >> -{ >> - struct svc_export *exp = container_of(ref, struct svc_export, >> h.ref); >> - >> - INIT_RCU_WORK(&exp->ex_rcu_work, svc_export_put_work); >> - queue_rcu_work(system_wq, &exp->ex_rcu_work); >> + kfree_rcu(exp, ex_rcu); >> } >> static int svc_export_upcall(struct cache_detail *cd, struct >> cache_head *h) >> diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h >> index 6f2fbaae01fa..4d92b99c1ffd 100644 >> --- a/fs/nfsd/export.h >> +++ b/fs/nfsd/export.h >> @@ -75,7 +75,7 @@ struct svc_export { >> u32 ex_layout_types; >> struct nfsd4_deviceid_map *ex_devid_map; >> struct cache_detail *cd; >> - struct rcu_work ex_rcu_work; >> + struct rcu_head ex_rcu; >> unsigned long ex_xprtsec_modes; >> struct export_stats *ex_stats; >> }; >> @@ -92,7 +92,7 @@ struct svc_expkey { >> u32 ek_fsid[6]; >> struct path ek_path; >> - struct rcu_work ek_rcu_work; >> + struct rcu_head ek_rcu; >> }; >> #define EX_ISSYNC(exp) (!((exp)->ex_flags & NFSEXP_ASYNC)) > >
diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c index eacafe46e3b6..aa4712362b3b 100644 --- a/fs/nfsd/export.c +++ b/fs/nfsd/export.c @@ -40,24 +40,15 @@ #define EXPKEY_HASHMAX (1 << EXPKEY_HASHBITS) #define EXPKEY_HASHMASK (EXPKEY_HASHMAX -1) -static void expkey_put_work(struct work_struct *work) +static void expkey_put(struct kref *ref) { - struct svc_expkey *key = - container_of(to_rcu_work(work), struct svc_expkey, ek_rcu_work); + struct svc_expkey *key = container_of(ref, struct svc_expkey, h.ref); if (test_bit(CACHE_VALID, &key->h.flags) && !test_bit(CACHE_NEGATIVE, &key->h.flags)) path_put(&key->ek_path); auth_domain_put(key->ek_client); - kfree(key); -} - -static void expkey_put(struct kref *ref) -{ - struct svc_expkey *key = container_of(ref, struct svc_expkey, h.ref); - - INIT_RCU_WORK(&key->ek_rcu_work, expkey_put_work); - queue_rcu_work(system_wq, &key->ek_rcu_work); + kfree_rcu(key, ek_rcu); } static int expkey_upcall(struct cache_detail *cd, struct cache_head *h) @@ -364,26 +355,16 @@ static void export_stats_destroy(struct export_stats *stats) EXP_STATS_COUNTERS_NUM); } -static void svc_export_put_work(struct work_struct *work) +static void svc_export_put(struct kref *ref) { - struct svc_export *exp = - container_of(to_rcu_work(work), struct svc_export, ex_rcu_work); - + struct svc_export *exp = container_of(ref, struct svc_export, h.ref); path_put(&exp->ex_path); auth_domain_put(exp->ex_client); nfsd4_fslocs_free(&exp->ex_fslocs); export_stats_destroy(exp->ex_stats); kfree(exp->ex_stats); kfree(exp->ex_uuid); - kfree(exp); -} - -static void svc_export_put(struct kref *ref) -{ - struct svc_export *exp = container_of(ref, struct svc_export, h.ref); - - INIT_RCU_WORK(&exp->ex_rcu_work, svc_export_put_work); - queue_rcu_work(system_wq, &exp->ex_rcu_work); + kfree_rcu(exp, ex_rcu); } static int svc_export_upcall(struct cache_detail *cd, struct cache_head *h) diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h index 6f2fbaae01fa..4d92b99c1ffd 100644 --- a/fs/nfsd/export.h +++ b/fs/nfsd/export.h @@ -75,7 +75,7 @@ struct svc_export { u32 ex_layout_types; struct nfsd4_deviceid_map *ex_devid_map; struct cache_detail *cd; - struct rcu_work ex_rcu_work; + struct rcu_head ex_rcu; unsigned long ex_xprtsec_modes; struct export_stats *ex_stats; }; @@ -92,7 +92,7 @@ struct svc_expkey { u32 ek_fsid[6]; struct path ek_path; - struct rcu_work ek_rcu_work; + struct rcu_head ek_rcu; }; #define EX_ISSYNC(exp) (!((exp)->ex_flags & NFSEXP_ASYNC))