| Message ID | 1311792048-12551-7-git-send-email-rees@umich.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2011-07-27 at 14:40 -0400, Jim Rees wrote: > From: Andy Adamson <andros@netapp.com> > > This gives layout driver a chance to cleanup structures they put in at > encode_layoutcommit. > > Signed-off-by: Andy Adamson <andros@netapp.com> > [fixup layout header pointer for layoutcommit] > Signed-off-by: Benny Halevy <bhalevy@panasas.com> > Signed-off-by: Benny Halevy <bhalevy@tonian.com> > --- > fs/nfs/nfs4proc.c | 1 + > fs/nfs/nfs4xdr.c | 1 + > fs/nfs/pnfs.c | 10 ++++++++++ > fs/nfs/pnfs.h | 5 +++++ > include/linux/nfs_xdr.h | 1 + > 5 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index e86de79..6cb84b4 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -5963,6 +5963,7 @@ static void nfs4_layoutcommit_release(void *calldata) > struct nfs4_layoutcommit_data *data = calldata; > struct pnfs_layout_segment *lseg, *tmp; > > + pnfs_cleanup_layoutcommit(data->args.inode, data); > /* Matched by references in pnfs_set_layoutcommit */ > list_for_each_entry_safe(lseg, tmp, &data->lseg_list, pls_lc_list) { > list_del_init(&lseg->pls_lc_list); > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > index f32dde9..ea2da30 100644 > --- a/fs/nfs/nfs4xdr.c > +++ b/fs/nfs/nfs4xdr.c > @@ -5596,6 +5596,7 @@ static int decode_layoutcommit(struct xdr_stream *xdr, > int status; > > status = decode_op_hdr(xdr, OP_LAYOUTCOMMIT); > + res->status = status; > if (status) > return status; > > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > index fbebd2a..3b20753 100644 > --- a/fs/nfs/pnfs.c > +++ b/fs/nfs/pnfs.c > @@ -1411,6 +1411,16 @@ pnfs_set_layoutcommit(struct nfs_write_data *wdata) > } > EXPORT_SYMBOL_GPL(pnfs_set_layoutcommit); > > +void pnfs_cleanup_layoutcommit(struct inode *inode, > + struct nfs4_layoutcommit_data *data) > +{ > + struct nfs_server *nfss = NFS_SERVER(inode); > + > + if (nfss->pnfs_curr_ld->cleanup_layoutcommit) > + nfss->pnfs_curr_ld->cleanup_layoutcommit(NFS_I(inode)->layout, > + data); > +} > + > /* > * For the LAYOUT4_NFSV4_1_FILES layout type, NFS_DATA_SYNC WRITEs and > * NFS_UNSTABLE WRITEs with a COMMIT to data servers must store enough > diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h > index bddd8b9..f271425 100644 > --- a/fs/nfs/pnfs.h > +++ b/fs/nfs/pnfs.h > @@ -113,6 +113,9 @@ struct pnfs_layoutdriver_type { > struct xdr_stream *xdr, > const struct nfs4_layoutreturn_args *args); > > + void (*cleanup_layoutcommit) (struct pnfs_layout_hdr *layoutid, > + struct nfs4_layoutcommit_data *data); > + > void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid, > struct xdr_stream *xdr, > const struct nfs4_layoutcommit_args *args); This really needs to go. We should have int (*layoutcommit)()... instead of 'encode' and 'cleanup' methods... > @@ -196,6 +199,8 @@ void pnfs_roc_release(struct inode *ino); > void pnfs_roc_set_barrier(struct inode *ino, u32 barrier); > bool pnfs_roc_drain(struct inode *ino, u32 *barrier); > void pnfs_set_layoutcommit(struct nfs_write_data *wdata); > +void pnfs_cleanup_layoutcommit(struct inode *inode, > + struct nfs4_layoutcommit_data *data); > int pnfs_layoutcommit_inode(struct inode *inode, bool sync); > int _pnfs_return_layout(struct inode *); > int pnfs_ld_write_done(struct nfs_write_data *); > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h > index 94f27e5..569ea5b 100644 > --- a/include/linux/nfs_xdr.h > +++ b/include/linux/nfs_xdr.h > @@ -269,6 +269,7 @@ struct nfs4_layoutcommit_res { > struct nfs_fattr *fattr; > const struct nfs_server *server; > struct nfs4_sequence_res seq_res; > + int status; > }; > > struct nfs4_layoutcommit_data {
On 07/27/2011 12:53 PM, Trond Myklebust wrote: >> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h >> index bddd8b9..f271425 100644 >> --- a/fs/nfs/pnfs.h >> +++ b/fs/nfs/pnfs.h >> @@ -113,6 +113,9 @@ struct pnfs_layoutdriver_type { >> struct xdr_stream *xdr, >> const struct nfs4_layoutreturn_args *args); >> >> + void (*cleanup_layoutcommit) (struct pnfs_layout_hdr *layoutid, >> + struct nfs4_layoutcommit_data *data); >> + >> void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid, >> struct xdr_stream *xdr, >> const struct nfs4_layoutcommit_args *args); > > This really needs to go. We should have > > int (*layoutcommit)()... > > instead of 'encode' and 'cleanup' methods... > Theoretically it is not possible because the blocks-layout protocol mandates different handling depending on the "error" response from the Server which will be received on RPC done. Though, Last I was diving into this code, The proper handling was missing. (It was a while back though things might have changed) Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-07-27 at 13:20 -0700, Boaz Harrosh wrote: > On 07/27/2011 12:53 PM, Trond Myklebust wrote: > >> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h > >> index bddd8b9..f271425 100644 > >> --- a/fs/nfs/pnfs.h > >> +++ b/fs/nfs/pnfs.h > >> @@ -113,6 +113,9 @@ struct pnfs_layoutdriver_type { > >> struct xdr_stream *xdr, > >> const struct nfs4_layoutreturn_args *args); > >> > >> + void (*cleanup_layoutcommit) (struct pnfs_layout_hdr *layoutid, > >> + struct nfs4_layoutcommit_data *data); > >> + > >> void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid, > >> struct xdr_stream *xdr, > >> const struct nfs4_layoutcommit_args *args); > > > > This really needs to go. We should have > > > > int (*layoutcommit)()... > > > > instead of 'encode' and 'cleanup' methods... > > > > Theoretically it is not possible because the blocks-layout protocol mandates > different handling depending on the "error" response from the Server which > will be received on RPC done. ???? If the blocks code is in charge of actually doing the RPC call, why would it not be able to perform its own error handling?
On 07/27/2011 01:25 PM, Trond Myklebust wrote: > On Wed, 2011-07-27 at 13:20 -0700, Boaz Harrosh wrote: >> On 07/27/2011 12:53 PM, Trond Myklebust wrote: >>>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h >>>> index bddd8b9..f271425 100644 >>>> --- a/fs/nfs/pnfs.h >>>> +++ b/fs/nfs/pnfs.h >>>> @@ -113,6 +113,9 @@ struct pnfs_layoutdriver_type { >>>> struct xdr_stream *xdr, >>>> const struct nfs4_layoutreturn_args *args); >>>> >>>> + void (*cleanup_layoutcommit) (struct pnfs_layout_hdr *layoutid, >>>> + struct nfs4_layoutcommit_data *data); >>>> + >>>> void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid, >>>> struct xdr_stream *xdr, >>>> const struct nfs4_layoutcommit_args *args); >>> >>> This really needs to go. We should have >>> >>> int (*layoutcommit)()... >>> >>> instead of 'encode' and 'cleanup' methods... >>> >> >> Theoretically it is not possible because the blocks-layout protocol mandates >> different handling depending on the "error" response from the Server which >> will be received on RPC done. > > ???? If the blocks code is in charge of actually doing the RPC call, why > would it not be able to perform its own error handling? > Is it? I thought it was the Generic layer that was Initiating the layoutcommit (From the pnfs_layoutcommit_inode called from nfs_write_inode) The LD only has a chance to encode the payload on rpc-setup and here the blocks code needs cleanup depending on the return-status of rpc-done [I do think that setup and done might be better names to reflect the rpc states) Thanks Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-07-27 at 13:42 -0700, Boaz Harrosh wrote: > On 07/27/2011 01:25 PM, Trond Myklebust wrote: > > On Wed, 2011-07-27 at 13:20 -0700, Boaz Harrosh wrote: > >> On 07/27/2011 12:53 PM, Trond Myklebust wrote: > >>>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h > >>>> index bddd8b9..f271425 100644 > >>>> --- a/fs/nfs/pnfs.h > >>>> +++ b/fs/nfs/pnfs.h > >>>> @@ -113,6 +113,9 @@ struct pnfs_layoutdriver_type { > >>>> struct xdr_stream *xdr, > >>>> const struct nfs4_layoutreturn_args *args); > >>>> > >>>> + void (*cleanup_layoutcommit) (struct pnfs_layout_hdr *layoutid, > >>>> + struct nfs4_layoutcommit_data *data); > >>>> + > >>>> void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid, > >>>> struct xdr_stream *xdr, > >>>> const struct nfs4_layoutcommit_args *args); > >>> > >>> This really needs to go. We should have > >>> > >>> int (*layoutcommit)()... > >>> > >>> instead of 'encode' and 'cleanup' methods... > >>> > >> > >> Theoretically it is not possible because the blocks-layout protocol mandates > >> different handling depending on the "error" response from the Server which > >> will be received on RPC done. > > > > ???? If the blocks code is in charge of actually doing the RPC call, why > > would it not be able to perform its own error handling? > > > > Is it? I thought it was the Generic layer that was Initiating the layoutcommit > (From the pnfs_layoutcommit_inode called from nfs_write_inode) > > The LD only has a chance to encode the payload on rpc-setup and here the blocks > code needs cleanup depending on the return-status of rpc-done > > [I do think that setup and done might be better names to reflect the rpc states) I'm suggesting replacing the version in the generic layer with per-layout-type variants. When the only thing that is common between the 3 variants is a couple of lines of xdr, then it doesn't make sense IMO to try to share.
On 07/27/2011 01:59 PM, Trond Myklebust wrote: > On Wed, 2011-07-27 at 13:42 -0700, Boaz Harrosh wrote: >> On 07/27/2011 01:25 PM, Trond Myklebust wrote: >>> On Wed, 2011-07-27 at 13:20 -0700, Boaz Harrosh wrote: >>>> On 07/27/2011 12:53 PM, Trond Myklebust wrote: >>>>>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h >>>>>> index bddd8b9..f271425 100644 >>>>>> --- a/fs/nfs/pnfs.h >>>>>> +++ b/fs/nfs/pnfs.h >>>>>> @@ -113,6 +113,9 @@ struct pnfs_layoutdriver_type { >>>>>> struct xdr_stream *xdr, >>>>>> const struct nfs4_layoutreturn_args *args); >>>>>> >>>>>> + void (*cleanup_layoutcommit) (struct pnfs_layout_hdr *layoutid, >>>>>> + struct nfs4_layoutcommit_data *data); >>>>>> + >>>>>> void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid, >>>>>> struct xdr_stream *xdr, >>>>>> const struct nfs4_layoutcommit_args *args); >>>>> >>>>> This really needs to go. We should have >>>>> >>>>> int (*layoutcommit)()... >>>>> >>>>> instead of 'encode' and 'cleanup' methods... >>>>> >>>> >>>> Theoretically it is not possible because the blocks-layout protocol mandates >>>> different handling depending on the "error" response from the Server which >>>> will be received on RPC done. >>> >>> ???? If the blocks code is in charge of actually doing the RPC call, why >>> would it not be able to perform its own error handling? >>> >> >> Is it? I thought it was the Generic layer that was Initiating the layoutcommit >> (From the pnfs_layoutcommit_inode called from nfs_write_inode) >> >> The LD only has a chance to encode the payload on rpc-setup and here the blocks >> code needs cleanup depending on the return-status of rpc-done >> >> [I do think that setup and done might be better names to reflect the rpc states) > > I'm suggesting replacing the version in the generic layer with > per-layout-type variants. When the only thing that is common between the > 3 variants is a couple of lines of xdr, then it doesn't make sense IMO > to try to share. > You lost me. What are you suggesting to replace? pnfs_layoutcommit_inode? nfs4_proc_layoutcommit? nfs4_xdr_enc_layoutcommit? At what level do you want to switch to a per LD handling. If I look at the all layoutcommit stack the actual xdr encoding is the least of the common code. The housekeeping is the most of it. I do not see a clear point in current code that we can make a clean cut. And that gives us a place that: 1. Already have an xdr buffer to encode into. 2. Also sees the return code from layoutcommit_done We used to allocated the layoutcommit buffer twice and that didn't solve that problem either. It is two stages of the rpc-state. I don't see how it can be briged Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index e86de79..6cb84b4 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -5963,6 +5963,7 @@ static void nfs4_layoutcommit_release(void *calldata) struct nfs4_layoutcommit_data *data = calldata; struct pnfs_layout_segment *lseg, *tmp; + pnfs_cleanup_layoutcommit(data->args.inode, data); /* Matched by references in pnfs_set_layoutcommit */ list_for_each_entry_safe(lseg, tmp, &data->lseg_list, pls_lc_list) { list_del_init(&lseg->pls_lc_list); diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index f32dde9..ea2da30 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -5596,6 +5596,7 @@ static int decode_layoutcommit(struct xdr_stream *xdr, int status; status = decode_op_hdr(xdr, OP_LAYOUTCOMMIT); + res->status = status; if (status) return status; diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c index fbebd2a..3b20753 100644 --- a/fs/nfs/pnfs.c +++ b/fs/nfs/pnfs.c @@ -1411,6 +1411,16 @@ pnfs_set_layoutcommit(struct nfs_write_data *wdata) } EXPORT_SYMBOL_GPL(pnfs_set_layoutcommit); +void pnfs_cleanup_layoutcommit(struct inode *inode, + struct nfs4_layoutcommit_data *data) +{ + struct nfs_server *nfss = NFS_SERVER(inode); + + if (nfss->pnfs_curr_ld->cleanup_layoutcommit) + nfss->pnfs_curr_ld->cleanup_layoutcommit(NFS_I(inode)->layout, + data); +} + /* * For the LAYOUT4_NFSV4_1_FILES layout type, NFS_DATA_SYNC WRITEs and * NFS_UNSTABLE WRITEs with a COMMIT to data servers must store enough diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h index bddd8b9..f271425 100644 --- a/fs/nfs/pnfs.h +++ b/fs/nfs/pnfs.h @@ -113,6 +113,9 @@ struct pnfs_layoutdriver_type { struct xdr_stream *xdr, const struct nfs4_layoutreturn_args *args); + void (*cleanup_layoutcommit) (struct pnfs_layout_hdr *layoutid, + struct nfs4_layoutcommit_data *data); + void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid, struct xdr_stream *xdr, const struct nfs4_layoutcommit_args *args); @@ -196,6 +199,8 @@ void pnfs_roc_release(struct inode *ino); void pnfs_roc_set_barrier(struct inode *ino, u32 barrier); bool pnfs_roc_drain(struct inode *ino, u32 *barrier); void pnfs_set_layoutcommit(struct nfs_write_data *wdata); +void pnfs_cleanup_layoutcommit(struct inode *inode, + struct nfs4_layoutcommit_data *data); int pnfs_layoutcommit_inode(struct inode *inode, bool sync); int _pnfs_return_layout(struct inode *); int pnfs_ld_write_done(struct nfs_write_data *); diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h index 94f27e5..569ea5b 100644 --- a/include/linux/nfs_xdr.h +++ b/include/linux/nfs_xdr.h @@ -269,6 +269,7 @@ struct nfs4_layoutcommit_res { struct nfs_fattr *fattr; const struct nfs_server *server; struct nfs4_sequence_res seq_res; + int status; }; struct nfs4_layoutcommit_data {