diff mbox series

[v1,05/13] NFS skip recovery of copy open on dest server

Message ID 20181019152905.32418-6-olga.kornievskaia@gmail.com (mailing list archive)
State New, archived
Headers show
Series server-side support for "inter" SSC copy | expand

Commit Message

Olga Kornievskaia Oct. 19, 2018, 3:28 p.m. UTC
From: Olga Kornievskaia <kolga@netapp.com>

Mark the open created for the source file on the destination
server. Then if this open is going thru a recovery, then fail
the recovery as we don't need to be recoving a "fake" open.
We need to fail the ongoing READs and vfs_copy_file_range().

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/nfs4_fs.h   |  1 +
 fs/nfs/nfs4file.c  |  1 +
 fs/nfs/nfs4state.c | 14 ++++++++++++++
 3 files changed, 16 insertions(+)

Comments

J. Bruce Fields Nov. 1, 2018, 8:19 p.m. UTC | #1
On Fri, Oct 19, 2018 at 11:28:57AM -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Mark the open created for the source file on the destination
> server. Then if this open is going thru a recovery, then fail
> the recovery as we don't need to be recoving a "fake" open.
> We need to fail the ongoing READs and vfs_copy_file_range().

Nothing to do with this patch, really, but:

I'm not always a stickler about the 80-character-column rule, but, man,
nfs4_reclaim_open_state is deeply nested, could that be broken up a
little?  Also I notice there's a printk("AGLO: ...") in there that you
probably didn't mean to leave upstream?

--b.

> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/nfs4_fs.h   |  1 +
>  fs/nfs/nfs4file.c  |  1 +
>  fs/nfs/nfs4state.c | 14 ++++++++++++++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index f229864..8b00c90 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -165,6 +165,7 @@ enum {
>  	NFS_STATE_CHANGE_WAIT,		/* A state changing operation is outstanding */
>  #ifdef CONFIG_NFS_V4_2
>  	NFS_CLNT_DST_SSC_COPY_STATE,    /* dst server open state on client*/
> +	NFS_SRV_SSC_COPY_STATE,		/* ssc state on the dst server */
>  #endif /* CONFIG_NFS_V4_2 */
>  };
>  
> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> index f82cb05..7909cd4 100644
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -309,6 +309,7 @@ struct file *
>  	if (ctx->state == NULL)
>  		goto out_stateowner;
>  
> +	set_bit(NFS_SRV_SSC_COPY_STATE, &ctx->state->flags);
>  	set_bit(NFS_OPEN_STATE, &ctx->state->flags);
>  	memcpy(&ctx->state->open_stateid.other, &stateid->other,
>  	       NFS4_STATEID_OTHER_SIZE);
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 62ae0fd..b0b82c6 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -1606,6 +1606,9 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
>  {
>  	struct nfs4_state *state;
>  	int status = 0;
> +#ifdef CONFIG_NFS_V4_2
> +	bool found_ssc_copy_state = false;
> +#endif /* CONFIG_NFS_V4_2 */
>  
>  	/* Note: we rely on the sp->so_states list being ordered 
>  	 * so that we always reclaim open(O_RDWR) and/or open(O_WRITE)
> @@ -1625,6 +1628,13 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
>  			continue;
>  		if (state->state == 0)
>  			continue;
> +#ifdef CONFIG_NFS_V4_2
> +		if (test_bit(NFS_SRV_SSC_COPY_STATE, &state->flags)) {
> +			nfs4_state_mark_recovery_failed(state, -EIO);
> +			found_ssc_copy_state = true;
> +			continue;
> +		}
> +#endif /* CONFIG_NFS_V4_2 */
>  		refcount_inc(&state->count);
>  		spin_unlock(&sp->so_lock);
>  		status = __nfs4_reclaim_open_state(sp, state, ops);
> @@ -1671,6 +1681,10 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
>  	}
>  	raw_write_seqcount_end(&sp->so_reclaim_seqcount);
>  	spin_unlock(&sp->so_lock);
> +#ifdef CONFIG_NFS_V4_2
> +	if (found_ssc_copy_state)
> +		return -EIO;
> +#endif /* CONFIG_NFS_V4_2 */
>  	return 0;
>  out_err:
>  	nfs4_put_open_state(state);
> -- 
> 1.8.3.1
Olga Kornievskaia Nov. 1, 2018, 8:38 p.m. UTC | #2
On Thu, Nov 1, 2018 at 4:20 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:28:57AM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Mark the open created for the source file on the destination
> > server. Then if this open is going thru a recovery, then fail
> > the recovery as we don't need to be recoving a "fake" open.
> > We need to fail the ongoing READs and vfs_copy_file_range().
>
> Nothing to do with this patch, really, but:

And because it not related to this patch, then I'm confused.

> I'm not always a stickler about the 80-character-column rule, but, man,
> nfs4_reclaim_open_state is deeply nested, could that be broken up a
> little?

really? it's nested? Anna reworked the reclaim state code and nesting
has gone done considerably. It's only 2 tabs in?

>  Also I notice there's a printk("AGLO: ...") in there that you
> probably didn't mean to leave upstream?

I don't believe I've ever had a printk() in the code. I did have a
leftover dprintk() and that has since been removed.

It looks like my attempt at providing you with the client side patches
might have created a problem where you are reviewing (now) an outdated
code. I never expected you to review the client side patches. However,
if you are so inclined then could you please just look at my latest
submission (v8) of the client patches as oppose to looking the version
submitted for the nfsd.

>
> --b.
>
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfs/nfs4_fs.h   |  1 +
> >  fs/nfs/nfs4file.c  |  1 +
> >  fs/nfs/nfs4state.c | 14 ++++++++++++++
> >  3 files changed, 16 insertions(+)
> >
> > diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> > index f229864..8b00c90 100644
> > --- a/fs/nfs/nfs4_fs.h
> > +++ b/fs/nfs/nfs4_fs.h
> > @@ -165,6 +165,7 @@ enum {
> >       NFS_STATE_CHANGE_WAIT,          /* A state changing operation is outstanding */
> >  #ifdef CONFIG_NFS_V4_2
> >       NFS_CLNT_DST_SSC_COPY_STATE,    /* dst server open state on client*/
> > +     NFS_SRV_SSC_COPY_STATE,         /* ssc state on the dst server */
> >  #endif /* CONFIG_NFS_V4_2 */
> >  };
> >
> > diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> > index f82cb05..7909cd4 100644
> > --- a/fs/nfs/nfs4file.c
> > +++ b/fs/nfs/nfs4file.c
> > @@ -309,6 +309,7 @@ struct file *
> >       if (ctx->state == NULL)
> >               goto out_stateowner;
> >
> > +     set_bit(NFS_SRV_SSC_COPY_STATE, &ctx->state->flags);
> >       set_bit(NFS_OPEN_STATE, &ctx->state->flags);
> >       memcpy(&ctx->state->open_stateid.other, &stateid->other,
> >              NFS4_STATEID_OTHER_SIZE);
> > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > index 62ae0fd..b0b82c6 100644
> > --- a/fs/nfs/nfs4state.c
> > +++ b/fs/nfs/nfs4state.c
> > @@ -1606,6 +1606,9 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
> >  {
> >       struct nfs4_state *state;
> >       int status = 0;
> > +#ifdef CONFIG_NFS_V4_2
> > +     bool found_ssc_copy_state = false;
> > +#endif /* CONFIG_NFS_V4_2 */
> >
> >       /* Note: we rely on the sp->so_states list being ordered
> >        * so that we always reclaim open(O_RDWR) and/or open(O_WRITE)
> > @@ -1625,6 +1628,13 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
> >                       continue;
> >               if (state->state == 0)
> >                       continue;
> > +#ifdef CONFIG_NFS_V4_2
> > +             if (test_bit(NFS_SRV_SSC_COPY_STATE, &state->flags)) {
> > +                     nfs4_state_mark_recovery_failed(state, -EIO);
> > +                     found_ssc_copy_state = true;
> > +                     continue;
> > +             }
> > +#endif /* CONFIG_NFS_V4_2 */
> >               refcount_inc(&state->count);
> >               spin_unlock(&sp->so_lock);
> >               status = __nfs4_reclaim_open_state(sp, state, ops);
> > @@ -1671,6 +1681,10 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
> >       }
> >       raw_write_seqcount_end(&sp->so_reclaim_seqcount);
> >       spin_unlock(&sp->so_lock);
> > +#ifdef CONFIG_NFS_V4_2
> > +     if (found_ssc_copy_state)
> > +             return -EIO;
> > +#endif /* CONFIG_NFS_V4_2 */
> >       return 0;
> >  out_err:
> >       nfs4_put_open_state(state);
> > --
> > 1.8.3.1
J. Bruce Fields Nov. 1, 2018, 9:24 p.m. UTC | #3
On Thu, Nov 01, 2018 at 04:38:43PM -0400, Olga Kornievskaia wrote:
> On Thu, Nov 1, 2018 at 4:20 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Fri, Oct 19, 2018 at 11:28:57AM -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > >
> > > Mark the open created for the source file on the destination
> > > server. Then if this open is going thru a recovery, then fail
> > > the recovery as we don't need to be recoving a "fake" open.
> > > We need to fail the ongoing READs and vfs_copy_file_range().
> >
> > Nothing to do with this patch, really, but:
> 
> And because it not related to this patch, then I'm confused.
> 
> > I'm not always a stickler about the 80-character-column rule, but, man,
> > nfs4_reclaim_open_state is deeply nested, could that be broken up a
> > little?
> 
> really? it's nested? Anna reworked the reclaim state code and nesting
> has gone done considerably. It's only 2 tabs in?
> 
> >  Also I notice there's a printk("AGLO: ...") in there that you
> > probably didn't mean to leave upstream?
> 
> I don't believe I've ever had a printk() in the code. I did have a
> leftover dprintk() and that has since been removed.

Oh, I see, this was all fixed recently upstream by Anna's cb7a8384dc02
"NFS: Split out the body of nfs4_reclaim_open_state()", ignore me.

--b.
diff mbox series

Patch

diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index f229864..8b00c90 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -165,6 +165,7 @@  enum {
 	NFS_STATE_CHANGE_WAIT,		/* A state changing operation is outstanding */
 #ifdef CONFIG_NFS_V4_2
 	NFS_CLNT_DST_SSC_COPY_STATE,    /* dst server open state on client*/
+	NFS_SRV_SSC_COPY_STATE,		/* ssc state on the dst server */
 #endif /* CONFIG_NFS_V4_2 */
 };
 
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index f82cb05..7909cd4 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -309,6 +309,7 @@  struct file *
 	if (ctx->state == NULL)
 		goto out_stateowner;
 
+	set_bit(NFS_SRV_SSC_COPY_STATE, &ctx->state->flags);
 	set_bit(NFS_OPEN_STATE, &ctx->state->flags);
 	memcpy(&ctx->state->open_stateid.other, &stateid->other,
 	       NFS4_STATEID_OTHER_SIZE);
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 62ae0fd..b0b82c6 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1606,6 +1606,9 @@  static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
 {
 	struct nfs4_state *state;
 	int status = 0;
+#ifdef CONFIG_NFS_V4_2
+	bool found_ssc_copy_state = false;
+#endif /* CONFIG_NFS_V4_2 */
 
 	/* Note: we rely on the sp->so_states list being ordered 
 	 * so that we always reclaim open(O_RDWR) and/or open(O_WRITE)
@@ -1625,6 +1628,13 @@  static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
 			continue;
 		if (state->state == 0)
 			continue;
+#ifdef CONFIG_NFS_V4_2
+		if (test_bit(NFS_SRV_SSC_COPY_STATE, &state->flags)) {
+			nfs4_state_mark_recovery_failed(state, -EIO);
+			found_ssc_copy_state = true;
+			continue;
+		}
+#endif /* CONFIG_NFS_V4_2 */
 		refcount_inc(&state->count);
 		spin_unlock(&sp->so_lock);
 		status = __nfs4_reclaim_open_state(sp, state, ops);
@@ -1671,6 +1681,10 @@  static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
 	}
 	raw_write_seqcount_end(&sp->so_reclaim_seqcount);
 	spin_unlock(&sp->so_lock);
+#ifdef CONFIG_NFS_V4_2
+	if (found_ssc_copy_state)
+		return -EIO;
+#endif /* CONFIG_NFS_V4_2 */
 	return 0;
 out_err:
 	nfs4_put_open_state(state);