On Mon, 2012-04-09 at 16:52 -0400, Fred Isaman wrote:
> Also create a commit_info structure to hold the bucket array and push
> it up from the lseg to the layout where it really belongs.
>
> While we are at it, fix a refcounting bug due to an (incorrect)
> implicit assumption that filelayout_scan_ds_commit_list always
> completely emptied the src list.
>
> This clarifies refcounting, removes the ugly find_only_write_lseg
> functions, and pushes the file layout commit code along on the path to
> supporting multiple lsegs.
>
> Signed-off-by: Fred Isaman <iisaman@xxxxxxxxxx>
> ---
> fs/nfs/nfs4filelayout.c | 150 +++++++++++++++++++++++++----------------------
> fs/nfs/nfs4filelayout.h | 20 ++++++-
> 2 files changed, 97 insertions(+), 73 deletions(-)
>
> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
> index 5acfd9e..0bbc88a 100644
> --- a/fs/nfs/nfs4filelayout.c
> +++ b/fs/nfs/nfs4filelayout.c
> @@ -650,7 +650,15 @@ filelayout_free_lseg(struct pnfs_layout_segment *lseg)
>
> dprintk("--> %s\n", __func__);
> nfs4_fl_put_deviceid(fl->dsaddr);
> - kfree(fl->commit_buckets);
> + /* This assumes a single RW lseg */
> + if (lseg->pls_range.iomode == IOMODE_RW) {
> + struct nfs4_filelayout *flo;
> +
> + flo = FILELAYOUT_FROM_HDR(lseg->pls_layout);
> + flo->commit_info.nbuckets = 0;
> + kfree(flo->commit_info.buckets);
> + flo->commit_info.buckets = NULL;
> + }
> _filelayout_free_lseg(fl);
> }
>
> @@ -681,19 +689,27 @@ filelayout_alloc_lseg(struct pnfs_layout_hdr *layoutid,
> * to filelayout_write_pagelist().
> * */
> if ((!fl->commit_through_mds) && (lgr->range.iomode == IOMODE_RW)) {
> + struct nfs4_filelayout *flo = FILELAYOUT_FROM_HDR(layoutid);
> int i;
> int size = (fl->stripe_type == STRIPE_SPARSE) ?
> fl->dsaddr->ds_num : fl->dsaddr->stripe_count;
>
> - fl->commit_buckets = kcalloc(size, sizeof(struct nfs4_fl_commit_bucket), gfp_flags);
> - if (!fl->commit_buckets) {
> + if (flo->commit_info.nbuckets != 0) {
> + /* Shouldn't happen if only one IOMODE_RW lseg */
> filelayout_free_lseg(&fl->generic_hdr);
> return NULL;
Is this really the correct thing to do? If we're dealing with multiple
IOMODE_RW lsegs, then surely we might find ourselves in a situation
where we might have to add new commit buckets.
Doesn't the above deserve a WARN_ON() and a FIXME comment?
> }
> - fl->number_of_buckets = size;
> + flo->commit_info.buckets = kcalloc(size,
> + sizeof(struct nfs4_fl_commit_bucket),
> + gfp_flags);
> + if (!flo->commit_info.buckets) {
> + filelayout_free_lseg(&fl->generic_hdr);
> + return NULL;
> + }
> + flo->commit_info.nbuckets = size;
> for (i = 0; i < size; i++) {
> - INIT_LIST_HEAD(&fl->commit_buckets[i].written);
> - INIT_LIST_HEAD(&fl->commit_buckets[i].committing);
> + INIT_LIST_HEAD(&flo->commit_info.buckets[i].written);
> + INIT_LIST_HEAD(&flo->commit_info.buckets[i].committing);
> }
The commit_info allocation and initialisation should probably be moved
into a different function if it is no longer part of the lseg.
That said, I'm having trouble seeing how this architecture can survive
in a future multiple-layout world. I'm thinking that since the
commit_buckets need to take a reference to the lseg, then maybe we
should keep the commit buckets tied to the lseg, and then let the commit
code iterate through the list of lsegs with something in their commit
buckets... What are your thoughts for how this might evolve?
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
