| Message ID | 20230606033212.1068823-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ceph: fix fscrypt_destroy_keyring use-after-free bug | expand |
xiubli@redhat.com writes: > From: Xiubo Li <xiubli@redhat.com> > > V2: > - Improve the code by switching to wait_for_completion_killable_timeout() > when umounting, at the same add one umount_timeout option. Instead of adding yet another (undocumented!) mount option, why not re-use the already existent 'mount_timeout' instead? It's already defined and kept in 'struct ceph_options', and the default value is defined with the same value you're using, in CEPH_MOUNT_TIMEOUT_DEFAULT. Cheers,
On 6/6/23 17:53, Luís Henriques wrote: > xiubli@redhat.com writes: > >> From: Xiubo Li <xiubli@redhat.com> >> >> V2: >> - Improve the code by switching to wait_for_completion_killable_timeout() >> when umounting, at the same add one umount_timeout option. > Instead of adding yet another (undocumented!) mount option, why not re-use > the already existent 'mount_timeout' instead? It's already defined and > kept in 'struct ceph_options', and the default value is defined with the > same value you're using, in CEPH_MOUNT_TIMEOUT_DEFAULT. This is for mount purpose. Is that okay to use the in umount case ? Thanks - Xiubo > Cheers,
Xiubo Li <xiubli@redhat.com> writes: > On 6/6/23 17:53, Luís Henriques wrote: >> xiubli@redhat.com writes: >> >>> From: Xiubo Li <xiubli@redhat.com> >>> >>> V2: >>> - Improve the code by switching to wait_for_completion_killable_timeout() >>> when umounting, at the same add one umount_timeout option. >> Instead of adding yet another (undocumented!) mount option, why not re-use >> the already existent 'mount_timeout' instead? It's already defined and >> kept in 'struct ceph_options', and the default value is defined with the >> same value you're using, in CEPH_MOUNT_TIMEOUT_DEFAULT. > > This is for mount purpose. Is that okay to use the in umount case ? Yeah, you're probably right. It's just that adding yet another knob for a corner case that probably will never be used and very few people will know about is never a good thing (IMO). Anyway, I think that at least this new mount option needs to be mentioned in 'Documentation/filesystems/ceph.rst'. Cheers,
On Wed, Jun 7, 2023 at 11:18 AM Luís Henriques <lhenriques@suse.de> wrote: > > Xiubo Li <xiubli@redhat.com> writes: > > > On 6/6/23 17:53, Luís Henriques wrote: > >> xiubli@redhat.com writes: > >> > >>> From: Xiubo Li <xiubli@redhat.com> > >>> > >>> V2: > >>> - Improve the code by switching to wait_for_completion_killable_timeout() > >>> when umounting, at the same add one umount_timeout option. > >> Instead of adding yet another (undocumented!) mount option, why not re-use > >> the already existent 'mount_timeout' instead? It's already defined and > >> kept in 'struct ceph_options', and the default value is defined with the > >> same value you're using, in CEPH_MOUNT_TIMEOUT_DEFAULT. > > > > This is for mount purpose. Is that okay to use the in umount case ? > > Yeah, you're probably right. It's just that adding yet another knob for a > corner case that probably will never be used and very few people will know > about is never a good thing (IMO). Anyway, I think that at least this new > mount option needs to be mentioned in 'Documentation/filesystems/ceph.rst'. It's OK and in fact preferrable to stick to mount_timeout to avoid new knobs. There is even a precedent for this: RBD uses mount_timeout both when waiting for the latest osdmap (on "rbd map") and when tearing down a watch (on "rbd unmap"). Thanks, Ilya
On 6/7/23 18:00, Ilya Dryomov wrote: > On Wed, Jun 7, 2023 at 11:18 AM Luís Henriques <lhenriques@suse.de> wrote: >> Xiubo Li <xiubli@redhat.com> writes: >> >>> On 6/6/23 17:53, Luís Henriques wrote: >>>> xiubli@redhat.com writes: >>>> >>>>> From: Xiubo Li <xiubli@redhat.com> >>>>> >>>>> V2: >>>>> - Improve the code by switching to wait_for_completion_killable_timeout() >>>>> when umounting, at the same add one umount_timeout option. >>>> Instead of adding yet another (undocumented!) mount option, why not re-use >>>> the already existent 'mount_timeout' instead? It's already defined and >>>> kept in 'struct ceph_options', and the default value is defined with the >>>> same value you're using, in CEPH_MOUNT_TIMEOUT_DEFAULT. >>> This is for mount purpose. Is that okay to use the in umount case ? >> Yeah, you're probably right. It's just that adding yet another knob for a >> corner case that probably will never be used and very few people will know >> about is never a good thing (IMO). Anyway, I think that at least this new >> mount option needs to be mentioned in 'Documentation/filesystems/ceph.rst'. > It's OK and in fact preferrable to stick to mount_timeout to avoid new > knobs. There is even a precedent for this: RBD uses mount_timeout both > when waiting for the latest osdmap (on "rbd map") and when tearing down > a watch (on "rbd unmap"). If so let's reuse the 'mount_timeout' here. Thanks - Xiubo > Thanks, > > Ilya >
From: Xiubo Li <xiubli@redhat.com> V2: - Improve the code by switching to wait_for_completion_killable_timeout() when umounting, at the same add one umount_timeout option. - Improve the inc/dec helpers for metadata/IO cases. Xiubo Li (2): ceph: drop the messages from MDS when unmounting ceph: just wait the osd requests' callbacks to finish when unmounting fs/ceph/addr.c | 10 +++++ fs/ceph/caps.c | 6 ++- fs/ceph/mds_client.c | 14 +++++-- fs/ceph/mds_client.h | 11 ++++- fs/ceph/quota.c | 14 +++---- fs/ceph/snap.c | 10 +++-- fs/ceph/super.c | 99 +++++++++++++++++++++++++++++++++++++++++++- fs/ceph/super.h | 7 ++++ 8 files changed, 154 insertions(+), 17 deletions(-)