All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ceph: discard r_new_inode if open O_CREAT opened existing inode
@ 2022-03-30 19:04 Jeff Layton
  2022-03-30 19:13 ` Jeff Layton
  2022-03-31  1:47 ` Xiubo Li
  0 siblings, 2 replies; 6+ messages in thread
From: Jeff Layton @ 2022-03-30 19:04 UTC (permalink / raw)
  To: ceph-devel; +Cc: idryomov, xiubli, Luís Henriques

When we do an unchecked create, we optimistically pre-create an inode
and populate it, including its fscrypt context. It's possible though
that we'll end up opening an existing inode, in which case the
precreated inode will have a crypto context that doesn't match the
existing data.

If we're issuing an O_CREAT open and find an existing inode, just
discard the precreated inode and create a new one to ensure the context
is properly set.

Cc: Luís Henriques <lhenriques@suse.de>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/mds_client.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 840a60b812fc..b03128fdbb07 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -3504,13 +3504,19 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
 	/* Must find target inode outside of mutexes to avoid deadlocks */
 	rinfo = &req->r_reply_info;
 	if ((err >= 0) && rinfo->head->is_target) {
-		struct inode *in;
+		struct inode *in = xchg(&req->r_new_inode, NULL);
 		struct ceph_vino tvino = {
 			.ino  = le64_to_cpu(rinfo->targeti.in->ino),
 			.snap = le64_to_cpu(rinfo->targeti.in->snapid)
 		};
 
-		in = ceph_get_inode(mdsc->fsc->sb, tvino, xchg(&req->r_new_inode, NULL));
+		/* If we ended up opening an existing inode, discard r_new_inode */
+		if (req->r_op == CEPH_MDS_OP_CREATE && !req->r_reply_info.has_create_ino) {
+			iput(in);
+			in = NULL;
+		}
+
+		in = ceph_get_inode(mdsc->fsc->sb, tvino, in);
 		if (IS_ERR(in)) {
 			err = PTR_ERR(in);
 			mutex_lock(&session->s_mutex);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: discard r_new_inode if open O_CREAT opened existing inode
  2022-03-30 19:04 [PATCH] ceph: discard r_new_inode if open O_CREAT opened existing inode Jeff Layton
@ 2022-03-30 19:13 ` Jeff Layton
  2022-03-31  9:49   ` Luís Henriques
  2022-03-31  1:47 ` Xiubo Li
  1 sibling, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2022-03-30 19:13 UTC (permalink / raw)
  To: ceph-devel; +Cc: idryomov, xiubli, Luís Henriques

On Wed, 2022-03-30 at 15:04 -0400, Jeff Layton wrote:
> When we do an unchecked create, we optimistically pre-create an inode
> and populate it, including its fscrypt context. It's possible though
> that we'll end up opening an existing inode, in which case the
> precreated inode will have a crypto context that doesn't match the
> existing data.
> 
> If we're issuing an O_CREAT open and find an existing inode, just
> discard the precreated inode and create a new one to ensure the context
> is properly set.
> 
> Cc: Luís Henriques <lhenriques@suse.de>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/mds_client.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 840a60b812fc..b03128fdbb07 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -3504,13 +3504,19 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
>  	/* Must find target inode outside of mutexes to avoid deadlocks */
>  	rinfo = &req->r_reply_info;
>  	if ((err >= 0) && rinfo->head->is_target) {
> -		struct inode *in;
> +		struct inode *in = xchg(&req->r_new_inode, NULL);
>  		struct ceph_vino tvino = {
>  			.ino  = le64_to_cpu(rinfo->targeti.in->ino),
>  			.snap = le64_to_cpu(rinfo->targeti.in->snapid)
>  		};
>  
> -		in = ceph_get_inode(mdsc->fsc->sb, tvino, xchg(&req->r_new_inode, NULL));
> +		/* If we ended up opening an existing inode, discard r_new_inode */
> +		if (req->r_op == CEPH_MDS_OP_CREATE && !req->r_reply_info.has_create_ino) {
> +			iput(in);
> +			in = NULL;
> +		}
> +
> +		in = ceph_get_inode(mdsc->fsc->sb, tvino, in);
>  		if (IS_ERR(in)) {
>  			err = PTR_ERR(in);
>  			mutex_lock(&session->s_mutex);

Forgot to mention that this one is for the fscrypt pile. This patch
fixes generic/595 for me.
-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: discard r_new_inode if open O_CREAT opened existing inode
  2022-03-30 19:04 [PATCH] ceph: discard r_new_inode if open O_CREAT opened existing inode Jeff Layton
  2022-03-30 19:13 ` Jeff Layton
@ 2022-03-31  1:47 ` Xiubo Li
  2022-03-31  9:53   ` Jeff Layton
  1 sibling, 1 reply; 6+ messages in thread
From: Xiubo Li @ 2022-03-31  1:47 UTC (permalink / raw)
  To: Jeff Layton; +Cc: idryomov, Luís Henriques, Ceph Development


On 3/31/22 3:04 AM, Jeff Layton wrote:
> When we do an unchecked create, we optimistically pre-create an inode
> and populate it, including its fscrypt context. It's possible though
> that we'll end up opening an existing inode, in which case the
> precreated inode will have a crypto context that doesn't match the
> existing data.
>
> If we're issuing an O_CREAT open and find an existing inode, just
> discard the precreated inode and create a new one to ensure the context
> is properly set.
>
> Cc: Luís Henriques <lhenriques@suse.de>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/ceph/mds_client.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 840a60b812fc..b03128fdbb07 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -3504,13 +3504,19 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
>   	/* Must find target inode outside of mutexes to avoid deadlocks */
>   	rinfo = &req->r_reply_info;
>   	if ((err >= 0) && rinfo->head->is_target) {
> -		struct inode *in;
> +		struct inode *in = xchg(&req->r_new_inode, NULL);
>   		struct ceph_vino tvino = {
>   			.ino  = le64_to_cpu(rinfo->targeti.in->ino),
>   			.snap = le64_to_cpu(rinfo->targeti.in->snapid)
>   		};
>   
> -		in = ceph_get_inode(mdsc->fsc->sb, tvino, xchg(&req->r_new_inode, NULL));
> +		/* If we ended up opening an existing inode, discard r_new_inode */
> +		if (req->r_op == CEPH_MDS_OP_CREATE && !req->r_reply_info.has_create_ino) {
> +			iput(in);

If the 'in' has a delegated ino, should we give it back here ?

-- Xiubo


> +			in = NULL;
> +		}
> +
> +		in = ceph_get_inode(mdsc->fsc->sb, tvino, in);
>   		if (IS_ERR(in)) {
>   			err = PTR_ERR(in);
>   			mutex_lock(&session->s_mutex);


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: discard r_new_inode if open O_CREAT opened existing inode
  2022-03-30 19:13 ` Jeff Layton
@ 2022-03-31  9:49   ` Luís Henriques
  0 siblings, 0 replies; 6+ messages in thread
From: Luís Henriques @ 2022-03-31  9:49 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, idryomov, xiubli

Jeff Layton <jlayton@kernel.org> writes:

> On Wed, 2022-03-30 at 15:04 -0400, Jeff Layton wrote:
>> When we do an unchecked create, we optimistically pre-create an inode
>> and populate it, including its fscrypt context. It's possible though
>> that we'll end up opening an existing inode, in which case the
>> precreated inode will have a crypto context that doesn't match the
>> existing data.
>> 
>> If we're issuing an O_CREAT open and find an existing inode, just
>> discard the precreated inode and create a new one to ensure the context
>> is properly set.
>> 
>> Cc: Luís Henriques <lhenriques@suse.de>
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>>  fs/ceph/mds_client.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>> 
>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>> index 840a60b812fc..b03128fdbb07 100644
>> --- a/fs/ceph/mds_client.c
>> +++ b/fs/ceph/mds_client.c
>> @@ -3504,13 +3504,19 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
>>  	/* Must find target inode outside of mutexes to avoid deadlocks */
>>  	rinfo = &req->r_reply_info;
>>  	if ((err >= 0) && rinfo->head->is_target) {
>> -		struct inode *in;
>> +		struct inode *in = xchg(&req->r_new_inode, NULL);
>>  		struct ceph_vino tvino = {
>>  			.ino  = le64_to_cpu(rinfo->targeti.in->ino),
>>  			.snap = le64_to_cpu(rinfo->targeti.in->snapid)
>>  		};
>>  
>> -		in = ceph_get_inode(mdsc->fsc->sb, tvino, xchg(&req->r_new_inode, NULL));
>> +		/* If we ended up opening an existing inode, discard r_new_inode */
>> +		if (req->r_op == CEPH_MDS_OP_CREATE && !req->r_reply_info.has_create_ino) {
>> +			iput(in);
>> +			in = NULL;
>> +		}
>> +
>> +		in = ceph_get_inode(mdsc->fsc->sb, tvino, in);
>>  		if (IS_ERR(in)) {
>>  			err = PTR_ERR(in);
>>  			mutex_lock(&session->s_mutex);
>
> Forgot to mention that this one is for the fscrypt pile. This patch
> fixes generic/595 for me.

Wow!  I must say it: the issue is very far from the places I was looking
at.  Thanks!

Reviewed-and-tested-by: Luís Henriques <lhenriques@suse.de>

Cheers,
-- 
Luís

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: discard r_new_inode if open O_CREAT opened existing inode
  2022-03-31  1:47 ` Xiubo Li
@ 2022-03-31  9:53   ` Jeff Layton
  2022-04-01  0:50     ` Xiubo Li
  0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2022-03-31  9:53 UTC (permalink / raw)
  To: Xiubo Li; +Cc: idryomov, Luís Henriques, Ceph Development

On Thu, 2022-03-31 at 09:47 +0800, Xiubo Li wrote:
> On 3/31/22 3:04 AM, Jeff Layton wrote:
> > When we do an unchecked create, we optimistically pre-create an inode
> > and populate it, including its fscrypt context. It's possible though
> > that we'll end up opening an existing inode, in which case the
> > precreated inode will have a crypto context that doesn't match the
> > existing data.
> > 
> > If we're issuing an O_CREAT open and find an existing inode, just
> > discard the precreated inode and create a new one to ensure the context
> > is properly set.
> > 
> > Cc: Luís Henriques <lhenriques@suse.de>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >   fs/ceph/mds_client.c | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 840a60b812fc..b03128fdbb07 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -3504,13 +3504,19 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
> >   	/* Must find target inode outside of mutexes to avoid deadlocks */
> >   	rinfo = &req->r_reply_info;
> >   	if ((err >= 0) && rinfo->head->is_target) {
> > -		struct inode *in;
> > +		struct inode *in = xchg(&req->r_new_inode, NULL);
> >   		struct ceph_vino tvino = {
> >   			.ino  = le64_to_cpu(rinfo->targeti.in->ino),
> >   			.snap = le64_to_cpu(rinfo->targeti.in->snapid)
> >   		};
> >   
> > -		in = ceph_get_inode(mdsc->fsc->sb, tvino, xchg(&req->r_new_inode, NULL));
> > +		/* If we ended up opening an existing inode, discard r_new_inode */
> > +		if (req->r_op == CEPH_MDS_OP_CREATE && !req->r_reply_info.has_create_ino) {
> > +			iput(in);
> 
> If the 'in' has a delegated ino, should we give it back here ?
> 
> -- Xiubo
> 
> 

This really shouldn't be a delegated ino. We only grab a delegated ino
if we're doing an async create, and in that case we should know that the
dentry doesn't exist and the create will succeed or fail without opening
the file.

It's probably worth throwing a warning though if we ever _do_ get a
delegated ino here. Let me consider how best to catch that situation.

> > +			in = NULL;
> > +		}
> > +
> > +		in = ceph_get_inode(mdsc->fsc->sb, tvino, in);
> >   		if (IS_ERR(in)) {
> >   			err = PTR_ERR(in);
> >   			mutex_lock(&session->s_mutex);
> 

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: discard r_new_inode if open O_CREAT opened existing inode
  2022-03-31  9:53   ` Jeff Layton
@ 2022-04-01  0:50     ` Xiubo Li
  0 siblings, 0 replies; 6+ messages in thread
From: Xiubo Li @ 2022-04-01  0:50 UTC (permalink / raw)
  To: Jeff Layton; +Cc: idryomov, Luís Henriques, Ceph Development


On 3/31/22 5:53 PM, Jeff Layton wrote:
> On Thu, 2022-03-31 at 09:47 +0800, Xiubo Li wrote:
>> On 3/31/22 3:04 AM, Jeff Layton wrote:
>>> When we do an unchecked create, we optimistically pre-create an inode
>>> and populate it, including its fscrypt context. It's possible though
>>> that we'll end up opening an existing inode, in which case the
>>> precreated inode will have a crypto context that doesn't match the
>>> existing data.
>>>
>>> If we're issuing an O_CREAT open and find an existing inode, just
>>> discard the precreated inode and create a new one to ensure the context
>>> is properly set.
>>>
>>> Cc: Luís Henriques <lhenriques@suse.de>
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>    fs/ceph/mds_client.c | 10 ++++++++--
>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>> index 840a60b812fc..b03128fdbb07 100644
>>> --- a/fs/ceph/mds_client.c
>>> +++ b/fs/ceph/mds_client.c
>>> @@ -3504,13 +3504,19 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
>>>    	/* Must find target inode outside of mutexes to avoid deadlocks */
>>>    	rinfo = &req->r_reply_info;
>>>    	if ((err >= 0) && rinfo->head->is_target) {
>>> -		struct inode *in;
>>> +		struct inode *in = xchg(&req->r_new_inode, NULL);
>>>    		struct ceph_vino tvino = {
>>>    			.ino  = le64_to_cpu(rinfo->targeti.in->ino),
>>>    			.snap = le64_to_cpu(rinfo->targeti.in->snapid)
>>>    		};
>>>    
>>> -		in = ceph_get_inode(mdsc->fsc->sb, tvino, xchg(&req->r_new_inode, NULL));
>>> +		/* If we ended up opening an existing inode, discard r_new_inode */
>>> +		if (req->r_op == CEPH_MDS_OP_CREATE && !req->r_reply_info.has_create_ino) {
>>> +			iput(in);
>> If the 'in' has a delegated ino, should we give it back here ?
>>
>> -- Xiubo
>>
>>
> This really shouldn't be a delegated ino. We only grab a delegated ino
> if we're doing an async create, and in that case we should know that the
> dentry doesn't exist and the create will succeed or fail without opening
> the file.

Yeah, right.

-- Xiubo

> It's probably worth throwing a warning though if we ever _do_ get a
> delegated ino here. Let me consider how best to catch that situation.
>
>>> +			in = NULL;
>>> +		}
>>> +
>>> +		in = ceph_get_inode(mdsc->fsc->sb, tvino, in);
>>>    		if (IS_ERR(in)) {
>>>    			err = PTR_ERR(in);
>>>    			mutex_lock(&session->s_mutex);
> Thanks,


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-01  0:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-30 19:04 [PATCH] ceph: discard r_new_inode if open O_CREAT opened existing inode Jeff Layton
2022-03-30 19:13 ` Jeff Layton
2022-03-31  9:49   ` Luís Henriques
2022-03-31  1:47 ` Xiubo Li
2022-03-31  9:53   ` Jeff Layton
2022-04-01  0:50     ` Xiubo Li

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.