Return-Path: Received: from mail-vw0-f46.google.com ([209.85.212.46]:32817 "EHLO mail-vw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753013Ab1FNPWD convert rfc822-to-8bit (ORCPT ); Tue, 14 Jun 2011 11:22:03 -0400 Received: by vws1 with SMTP id 1so4533895vws.19 for ; Tue, 14 Jun 2011 08:22:02 -0700 (PDT) In-Reply-To: <4DF779D0.8030501@gmail.com> References: <7f60aa4b7494834319738eb61a05057bc86a498d.1307921138.git.rees@umich.edu> <4DF779D0.8030501@gmail.com> From: Peng Tao Date: Tue, 14 Jun 2011 23:21:39 +0800 Message-ID: Subject: Re: [PATCH 06/34] pnfs: cleanup_layoutcommit To: Benny Halevy Cc: Jim Rees , linux-nfs@vger.kernel.org, peter honeyman Content-Type: text/plain; charset=UTF-8 Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On Tue, Jun 14, 2011 at 11:10 PM, Benny Halevy wrote: > On 2011-06-12 19:44, Jim Rees wrote: >> From: Peng Tao >> >> This gives layout driver a chance to cleanup structures they put in. >> Also ensure layoutcommit does not commit more than isize, as block layout >> driver may dirty pages beyond EOF. > > let's separate the latter matter into a different patch so we can > discuss the problem and the solution orthogonally to cleanup_layoutcommit. > >> >> Signed-off-by: Andy Adamson >> [fixup layout header pointer for layoutcommit] >> Signed-off-by: Benny Halevy >> Signed-off-by: Peng Tao >> --- >>  fs/nfs/nfs4proc.c       |    1 + >>  fs/nfs/nfs4xdr.c        |    3 ++- >>  fs/nfs/pnfs.c           |   15 +++++++++++++++ >>  fs/nfs/pnfs.h           |    4 ++++ >>  include/linux/nfs_xdr.h |    1 + >>  5 files changed, 23 insertions(+), 1 deletions(-) >> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index 5246db8..e27a648 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -5890,6 +5890,7 @@ static void nfs4_layoutcommit_release(void *calldata) >>  { >>       struct nfs4_layoutcommit_data *data = calldata; >> >> +     pnfs_cleanup_layoutcommit(data->args.inode, data); >>       /* Matched by references in pnfs_set_layoutcommit */ >>       put_lseg(data->lseg); >>       put_rpccred(data->cred); >> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c >> index fdcbd8f..57295d1 100644 >> --- a/fs/nfs/nfs4xdr.c >> +++ b/fs/nfs/nfs4xdr.c >> @@ -1963,7 +1963,7 @@ encode_layoutcommit(struct xdr_stream *xdr, >>       *p++ = cpu_to_be32(OP_LAYOUTCOMMIT); >>       /* Only whole file layouts */ >>       p = xdr_encode_hyper(p, 0); /* offset */ >> -     p = xdr_encode_hyper(p, NFS4_MAX_UINT64); /* length */ >> +     p = xdr_encode_hyper(p, args->lastbytewritten+1); /* length */ > > This is unrelated to this particular patch and it should be discussed separately. > (and dropped altogether :) > >>       *p++ = cpu_to_be32(0); /* reclaim */ >>       p = xdr_encode_opaque_fixed(p, args->stateid.data, NFS4_STATEID_SIZE); >>       *p++ = cpu_to_be32(1); /* newoffset = TRUE */ >> @@ -5467,6 +5467,7 @@ static int decode_layoutcommit(struct xdr_stream *xdr, >>       int status; >> >>       status = decode_op_hdr(xdr, OP_LAYOUTCOMMIT); >> +     res->status = status; > > What is res->status used for? block layout driver use it to determine if it should clean up commiting extent list or put them back to dirty extent list (which is probably wrong but it remains to be seen when layoutcommit error handling for layoutcommit is implemented in generic layer). > >>       if (status) >>               return status; >> >> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c >> index e693718..48a06a1 100644 >> --- a/fs/nfs/pnfs.c >> +++ b/fs/nfs/pnfs.c >> @@ -1248,6 +1248,7 @@ pnfs_set_layoutcommit(struct nfs_write_data *wdata) >>  { >>       struct nfs_inode *nfsi = NFS_I(wdata->inode); >>       loff_t end_pos = wdata->mds_offset + wdata->res.count; >> +     loff_t isize = i_size_read(wdata->inode); >>       bool mark_as_dirty = false; >> >>       spin_lock(&nfsi->vfs_inode.i_lock); >> @@ -1261,9 +1262,13 @@ pnfs_set_layoutcommit(struct nfs_write_data *wdata) >>               dprintk("%s: Set layoutcommit for inode %lu ", >>                       __func__, wdata->inode->i_ino); >>       } >> +     if (end_pos > isize) >> +             end_pos = isize; >>       if (end_pos > wdata->lseg->pls_end_pos) >>               wdata->lseg->pls_end_pos = end_pos; >>       spin_unlock(&nfsi->vfs_inode.i_lock); >> +     dprintk("%s: lseg %p end_pos %llu\n", >> +             __func__, wdata->lseg, wdata->lseg->pls_end_pos); >> >>       /* if pnfs_layoutcommit_inode() runs between inode locks, the next one >>        * will be a noop because NFS_INO_LAYOUTCOMMIT will not be set */ >> @@ -1272,6 +1277,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); >> +} >> + >>  void pnfs_free_fsdata(struct pnfs_fsdata *fsdata) >>  { >>       /* lseg refcounting handled directly in nfs_write_end */ >> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h >> index 525ec55..5048898 100644 >> --- a/fs/nfs/pnfs.h >> +++ b/fs/nfs/pnfs.h >> @@ -127,6 +127,9 @@ struct pnfs_layoutdriver_type { >>                                    struct xdr_stream *xdr,it: >>                                    const struct nfs4_layoutreturn_args *args); >> >> +        void (*cleanup_layoutcommit) (struct pnfs_layout_hdr *layoutid, >> +                                      struct nfs4_layoutcommit_data *data); >> + > > nit: whitespace cleanup required... > >>       void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid, >>                                    struct xdr_stream *xdr, >>                                    const struct nfs4_layoutcommit_args *args); >> @@ -213,6 +216,7 @@ 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 a9c43ba..2c3ffda 100644 >> --- a/include/linux/nfs_xdr.h >> +++ b/include/linux/nfs_xdr.h >> @@ -270,6 +270,7 @@ struct nfs4_layoutcommit_res { >>       struct nfs_fattr *fattr; >>       const struct nfs_server *server; >>       struct nfs4_sequence_res seq_res; >> +     int status; > > This seems to be unused in this patch... > > Benny > >>  }; >> >>  struct nfs4_layoutcommit_data { > -- > 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 > -- Thanks, -Bergwolf