diff mbox series

mds: update dentry lease for async create

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

Commit Message

Yan, Zheng March 4, 2020, 1:22 p.m. UTC
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(+)

Comments

Jeff Layton March 4, 2020, 1:55 p.m. UTC | #1
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!
Jeff Layton March 4, 2020, 2:05 p.m. UTC | #2
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?
Yan, Zheng March 4, 2020, 2:26 p.m. UTC | #3
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
Jeff Layton March 4, 2020, 2:50 p.m. UTC | #4
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 mbox series

Patch

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;