| Message ID | 1408637375-11343-9-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/21/2014 12:09 PM, Christoph Hellwig wrote: > If a layout driver keeps per-inode state outside of the layout segments it > needs to be notified of any layout returns or recalls on an inode, and not > just about the freeing of layout segments. Add a method to acomplish this, > which will allow the block layout driver to handle the case of truncated > and re-expanded files properly. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/nfs/callback_proc.c | 6 +++++- > fs/nfs/pnfs.c | 10 ++++++++++ > fs/nfs/pnfs.h | 3 +++ > 3 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c > index bf017b0..86541e0 100644 > --- a/fs/nfs/callback_proc.c > +++ b/fs/nfs/callback_proc.c > @@ -179,8 +179,12 @@ static u32 initiate_file_draining(struct nfs_client *clp, > &args->cbl_range)) { > need_commit = true; > rv = NFS4ERR_DELAY; > + } else { > + if (NFS_SERVER(ino)->pnfs_curr_ld->return_range) { > + NFS_SERVER(ino)->pnfs_curr_ld->return_range(lo, > + &args->cbl_range); > + } > } Is there a reason you're nesting the else-if here? Anna > - > pnfs_set_layout_stateid(lo, &args->cbl_stateid, true); > spin_unlock(&ino->i_lock); > pnfs_free_lseg_list(&free_me_list); > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > index bce7f1b..e481d1c 100644 > --- a/fs/nfs/pnfs.c > +++ b/fs/nfs/pnfs.c > @@ -857,6 +857,16 @@ _pnfs_return_layout(struct inode *ino) > empty = list_empty(&lo->plh_segs); > pnfs_clear_layoutcommit(ino, &tmp_list); > pnfs_mark_matching_lsegs_invalid(lo, &tmp_list, NULL); > + > + if (NFS_SERVER(ino)->pnfs_curr_ld->return_range) { > + struct pnfs_layout_range range = { > + .iomode = IOMODE_ANY, > + .offset = 0, > + .length = NFS4_MAX_UINT64, > + }; > + NFS_SERVER(ino)->pnfs_curr_ld->return_range(lo, &range); > + } > + > /* Don't send a LAYOUTRETURN if list was initially empty */ > if (empty) { > spin_unlock(&ino->i_lock); > diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h > index 302b279..044c071 100644 > --- a/fs/nfs/pnfs.h > +++ b/fs/nfs/pnfs.h > @@ -94,6 +94,9 @@ struct pnfs_layoutdriver_type { > struct pnfs_layout_segment * (*alloc_lseg) (struct pnfs_layout_hdr *layoutid, struct nfs4_layoutget_res *lgr, gfp_t gfp_flags); > void (*free_lseg) (struct pnfs_layout_segment *lseg); > > + void (*return_range) (struct pnfs_layout_hdr *lo, > + struct pnfs_layout_range *range); > + > /* test for nfs page cache coalescing */ > const struct nfs_pageio_ops *pg_read_ops; > const struct nfs_pageio_ops *pg_write_ops; -- 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 Mon, Aug 25, 2014 at 09:50:34AM -0400, Anna Schumaker wrote: > > + } else { > > + if (NFS_SERVER(ino)->pnfs_curr_ld->return_range) { > > + NFS_SERVER(ino)->pnfs_curr_ld->return_range(lo, > > + &args->cbl_range); > > + } > > } > Is there a reason you're nesting the else-if here? To catch the intent - the first two clauses find excuses why we can't return quite yet, while this if is for an optional feature in the actual return path. If I wasn't updating but newly writing the function I'd actually do something like: ... rv = NFS4ERR_DELAY; if (test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags)) goto out_set_stateid; if (pnfs_mark_matching_lsegs_invalid(lo, &free_me_list, &args->cbl_range)) { need_commit = true; goto out_set_stateid; } rv = NFS4ERR_NOMATCHING_LAYOUT; if (NFS_SERVER(ino)->pnfs_curr_ld->return_range) { NFS_SERVER(ino)->pnfs_curr_ld->return_range(lo, &args->cbl_range); } out_set_stateid: ... -- 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 08/25/2014 10:09 AM, Christoph Hellwig wrote: > On Mon, Aug 25, 2014 at 09:50:34AM -0400, Anna Schumaker wrote: >>> + } else { >>> + if (NFS_SERVER(ino)->pnfs_curr_ld->return_range) { >>> + NFS_SERVER(ino)->pnfs_curr_ld->return_range(lo, >>> + &args->cbl_range); >>> + } >>> } >> Is there a reason you're nesting the else-if here? > > To catch the intent - the first two clauses find excuses why we can't return > quite yet, while this if is for an optional feature in the actual return > path. If I wasn't updating but newly writing the function I'd actually > do something like: I'm a fan of nice looking code, and I like what you have below better. Can you arrange things to end up in this state? Or maybe send a cleanup patch after? Anna > > ... > > rv = NFS4ERR_DELAY; > if (test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags)) > goto out_set_stateid; > > if (pnfs_mark_matching_lsegs_invalid(lo, &free_me_list, > &args->cbl_range)) { > need_commit = true; > goto out_set_stateid; > } > > rv = NFS4ERR_NOMATCHING_LAYOUT; > if (NFS_SERVER(ino)->pnfs_curr_ld->return_range) { > NFS_SERVER(ino)->pnfs_curr_ld->return_range(lo, > &args->cbl_range); > } > > out_set_stateid: > ... > -- 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 Mon, Aug 25, 2014 at 10:17:24AM -0400, Anna Schumaker wrote: > > To catch the intent - the first two clauses find excuses why we can't return > > quite yet, while this if is for an optional feature in the actual return > > path. If I wasn't updating but newly writing the function I'd actually > > do something like: > > I'm a fan of nice looking code, and I like what you have below better. Can you arrange things to end up in this state? Or maybe send a cleanup patch after? I'll send a cleanup later, that is unless I need to respin the series in this area anyway for Boaz different layoutcommit on recall proposal. -- 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 Thu, Aug 21, 2014 at 9:09 AM, Christoph Hellwig <hch@lst.de> wrote: > If a layout driver keeps per-inode state outside of the layout segments it > needs to be notified of any layout returns or recalls on an inode, and not > just about the freeing of layout segments. Add a method to acomplish this, > which will allow the block layout driver to handle the case of truncated > and re-expanded files properly. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/nfs/callback_proc.c | 6 +++++- > fs/nfs/pnfs.c | 10 ++++++++++ > fs/nfs/pnfs.h | 3 +++ > 3 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c > index bf017b0..86541e0 100644 > --- a/fs/nfs/callback_proc.c > +++ b/fs/nfs/callback_proc.c > @@ -179,8 +179,12 @@ static u32 initiate_file_draining(struct nfs_client *clp, > &args->cbl_range)) { > need_commit = true; > rv = NFS4ERR_DELAY; > + } else { > + if (NFS_SERVER(ino)->pnfs_curr_ld->return_range) { > + NFS_SERVER(ino)->pnfs_curr_ld->return_range(lo, > + &args->cbl_range); > + } > } > - > pnfs_set_layout_stateid(lo, &args->cbl_stateid, true); > spin_unlock(&ino->i_lock); > pnfs_free_lseg_list(&free_me_list); > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > index bce7f1b..e481d1c 100644 > --- a/fs/nfs/pnfs.c > +++ b/fs/nfs/pnfs.c > @@ -857,6 +857,16 @@ _pnfs_return_layout(struct inode *ino) > empty = list_empty(&lo->plh_segs); > pnfs_clear_layoutcommit(ino, &tmp_list); > pnfs_mark_matching_lsegs_invalid(lo, &tmp_list, NULL); > + > + if (NFS_SERVER(ino)->pnfs_curr_ld->return_range) { > + struct pnfs_layout_range range = { > + .iomode = IOMODE_ANY, > + .offset = 0, > + .length = NFS4_MAX_UINT64, > + }; > + NFS_SERVER(ino)->pnfs_curr_ld->return_range(lo, &range); > + } > + > /* Don't send a LAYOUTRETURN if list was initially empty */ > if (empty) { > spin_unlock(&ino->i_lock); > diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h > index 302b279..044c071 100644 > --- a/fs/nfs/pnfs.h > +++ b/fs/nfs/pnfs.h > @@ -94,6 +94,9 @@ struct pnfs_layoutdriver_type { > struct pnfs_layout_segment * (*alloc_lseg) (struct pnfs_layout_hdr *layoutid, struct nfs4_layoutget_res *lgr, gfp_t gfp_flags); > void (*free_lseg) (struct pnfs_layout_segment *lseg); > > + void (*return_range) (struct pnfs_layout_hdr *lo, > + struct pnfs_layout_range *range); > + > /* test for nfs page cache coalescing */ > const struct nfs_pageio_ops *pg_read_ops; > const struct nfs_pageio_ops *pg_write_ops; Holding off applying this patch for now until we figure out the right behaviour for "[PATCH 03/19] pnfs: force a layout commit when encountering busy segments during recall". There is a small dependency in initiate_file_draining().
diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c index bf017b0..86541e0 100644 --- a/fs/nfs/callback_proc.c +++ b/fs/nfs/callback_proc.c @@ -179,8 +179,12 @@ static u32 initiate_file_draining(struct nfs_client *clp, &args->cbl_range)) { need_commit = true; rv = NFS4ERR_DELAY; + } else { + if (NFS_SERVER(ino)->pnfs_curr_ld->return_range) { + NFS_SERVER(ino)->pnfs_curr_ld->return_range(lo, + &args->cbl_range); + } } - pnfs_set_layout_stateid(lo, &args->cbl_stateid, true); spin_unlock(&ino->i_lock); pnfs_free_lseg_list(&free_me_list); diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c index bce7f1b..e481d1c 100644 --- a/fs/nfs/pnfs.c +++ b/fs/nfs/pnfs.c @@ -857,6 +857,16 @@ _pnfs_return_layout(struct inode *ino) empty = list_empty(&lo->plh_segs); pnfs_clear_layoutcommit(ino, &tmp_list); pnfs_mark_matching_lsegs_invalid(lo, &tmp_list, NULL); + + if (NFS_SERVER(ino)->pnfs_curr_ld->return_range) { + struct pnfs_layout_range range = { + .iomode = IOMODE_ANY, + .offset = 0, + .length = NFS4_MAX_UINT64, + }; + NFS_SERVER(ino)->pnfs_curr_ld->return_range(lo, &range); + } + /* Don't send a LAYOUTRETURN if list was initially empty */ if (empty) { spin_unlock(&ino->i_lock); diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h index 302b279..044c071 100644 --- a/fs/nfs/pnfs.h +++ b/fs/nfs/pnfs.h @@ -94,6 +94,9 @@ struct pnfs_layoutdriver_type { struct pnfs_layout_segment * (*alloc_lseg) (struct pnfs_layout_hdr *layoutid, struct nfs4_layoutget_res *lgr, gfp_t gfp_flags); void (*free_lseg) (struct pnfs_layout_segment *lseg); + void (*return_range) (struct pnfs_layout_hdr *lo, + struct pnfs_layout_range *range); + /* test for nfs page cache coalescing */ const struct nfs_pageio_ops *pg_read_ops; const struct nfs_pageio_ops *pg_write_ops;
If a layout driver keeps per-inode state outside of the layout segments it needs to be notified of any layout returns or recalls on an inode, and not just about the freeing of layout segments. Add a method to acomplish this, which will allow the block layout driver to handle the case of truncated and re-expanded files properly. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/nfs/callback_proc.c | 6 +++++- fs/nfs/pnfs.c | 10 ++++++++++ fs/nfs/pnfs.h | 3 +++ 3 files changed, 18 insertions(+), 1 deletion(-)