| Message ID | 20200304132220.41238-1-zyan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mds: update dentry lease for async create | expand |
On Wed, 2020-03-04 at 21:22 +0800, Yan, Zheng wrote: > Otherwise ceph_d_delete() may return 1 for the dentry, which makes > dput() prune the dentry and clear parent dir's complete flag. > > Signed-off-by: "Yan, Zheng" <zyan@redhat.com> > --- > fs/ceph/file.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index 53321bf517c2..671b141aedfe 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -480,6 +480,9 @@ static int try_prep_async_create(struct inode *dir, struct dentry *dentry, > if (d_in_lookup(dentry)) { > if (!__ceph_dir_is_complete(ci)) > goto no_async; > + spin_lock(&dentry->d_lock); > + di->lease_shared_gen = atomic_read(&ci->i_shared_gen); > + spin_unlock(&dentry->d_lock); > } else if (atomic_read(&ci->i_shared_gen) != > READ_ONCE(di->lease_shared_gen)) { > goto no_async; Good catch, merged into testing (with small update to changelog s/mds:/ceph:/) Thanks!
On Wed, 2020-03-04 at 08:55 -0500, Jeff Layton wrote: > On Wed, 2020-03-04 at 21:22 +0800, Yan, Zheng wrote: > > Otherwise ceph_d_delete() may return 1 for the dentry, which makes > > dput() prune the dentry and clear parent dir's complete flag. > > > > Signed-off-by: "Yan, Zheng" <zyan@redhat.com> > > --- > > fs/ceph/file.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > > index 53321bf517c2..671b141aedfe 100644 > > --- a/fs/ceph/file.c > > +++ b/fs/ceph/file.c > > @@ -480,6 +480,9 @@ static int try_prep_async_create(struct inode *dir, struct dentry *dentry, > > if (d_in_lookup(dentry)) { > > if (!__ceph_dir_is_complete(ci)) > > goto no_async; > > + spin_lock(&dentry->d_lock); > > + di->lease_shared_gen = atomic_read(&ci->i_shared_gen); > > + spin_unlock(&dentry->d_lock); > > } else if (atomic_read(&ci->i_shared_gen) != > > READ_ONCE(di->lease_shared_gen)) { > > goto no_async; > > Good catch, merged into testing (with small update to changelog > s/mds:/ceph:/) > One related comment though. This patch implies that lease_shared_gen is protected by the d_lock, but it's not held when it's assigned in ceph_lookup. Should it be?
On 3/4/20 10:05 PM, Jeff Layton wrote: > On Wed, 2020-03-04 at 08:55 -0500, Jeff Layton wrote: >> On Wed, 2020-03-04 at 21:22 +0800, Yan, Zheng wrote: >>> Otherwise ceph_d_delete() may return 1 for the dentry, which makes >>> dput() prune the dentry and clear parent dir's complete flag. >>> >>> Signed-off-by: "Yan, Zheng" <zyan@redhat.com> >>> --- >>> fs/ceph/file.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c >>> index 53321bf517c2..671b141aedfe 100644 >>> --- a/fs/ceph/file.c >>> +++ b/fs/ceph/file.c >>> @@ -480,6 +480,9 @@ static int try_prep_async_create(struct inode *dir, struct dentry *dentry, >>> if (d_in_lookup(dentry)) { >>> if (!__ceph_dir_is_complete(ci)) >>> goto no_async; >>> + spin_lock(&dentry->d_lock); >>> + di->lease_shared_gen = atomic_read(&ci->i_shared_gen); >>> + spin_unlock(&dentry->d_lock); >>> } else if (atomic_read(&ci->i_shared_gen) != >>> READ_ONCE(di->lease_shared_gen)) { >>> goto no_async; >> >> Good catch, merged into testing (with small update to changelog >> s/mds:/ceph:/) >> > > One related comment though. This patch implies that lease_shared_gen is > protected by the d_lock, but it's not held when it's assigned in > ceph_lookup. Should it be? > we only assign and compare lease_shared_gen, I think it doesn't matter if it's protected by d_lock or not
On Wed, 2020-03-04 at 22:26 +0800, Yan, Zheng wrote: > On 3/4/20 10:05 PM, Jeff Layton wrote: > > On Wed, 2020-03-04 at 08:55 -0500, Jeff Layton wrote: > > > On Wed, 2020-03-04 at 21:22 +0800, Yan, Zheng wrote: > > > > Otherwise ceph_d_delete() may return 1 for the dentry, which makes > > > > dput() prune the dentry and clear parent dir's complete flag. > > > > > > > > Signed-off-by: "Yan, Zheng" <zyan@redhat.com> > > > > --- > > > > fs/ceph/file.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > > > > index 53321bf517c2..671b141aedfe 100644 > > > > --- a/fs/ceph/file.c > > > > +++ b/fs/ceph/file.c > > > > @@ -480,6 +480,9 @@ static int try_prep_async_create(struct inode *dir, struct dentry *dentry, > > > > if (d_in_lookup(dentry)) { > > > > if (!__ceph_dir_is_complete(ci)) > > > > goto no_async; > > > > + spin_lock(&dentry->d_lock); > > > > + di->lease_shared_gen = atomic_read(&ci->i_shared_gen); > > > > + spin_unlock(&dentry->d_lock); > > > > } else if (atomic_read(&ci->i_shared_gen) != > > > > READ_ONCE(di->lease_shared_gen)) { > > > > goto no_async; > > > > > > Good catch, merged into testing (with small update to changelog > > > s/mds:/ceph:/) > > > > > > > One related comment though. This patch implies that lease_shared_gen is > > protected by the d_lock, but it's not held when it's assigned in > > ceph_lookup. Should it be? > > > > we only assign and compare lease_shared_gen, I think it doesn't matter > if it's protected by d_lock or not > That's the case here too. Do we need the d_lock in try_prep_async_create? Maybe lease_shared_gen should also be an atomic_t?
diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 53321bf517c2..671b141aedfe 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -480,6 +480,9 @@ static int try_prep_async_create(struct inode *dir, struct dentry *dentry, if (d_in_lookup(dentry)) { if (!__ceph_dir_is_complete(ci)) goto no_async; + spin_lock(&dentry->d_lock); + di->lease_shared_gen = atomic_read(&ci->i_shared_gen); + spin_unlock(&dentry->d_lock); } else if (atomic_read(&ci->i_shared_gen) != READ_ONCE(di->lease_shared_gen)) { goto no_async;
Otherwise ceph_d_delete() may return 1 for the dentry, which makes dput() prune the dentry and clear parent dir's complete flag. Signed-off-by: "Yan, Zheng" <zyan@redhat.com> --- fs/ceph/file.c | 3 +++ 1 file changed, 3 insertions(+)