[PATCH 0/2] pnfs-submit Re-initialize pnfs_layout_type when segs list is empty

[PATCH 0/2] pnfs-submit Re-initialize pnfs_layout_type when segs list is empty

[andros]

e keep the nfs_inode->layout when the segs list is empty, and we remove it
from the nfs_client cl_layouts list, but we fail to reset the other fields.
Re-initialize the layout (all except for the refcount) so that the next
layoutget with potentially new deviceid.... sets the layout fields.

Note: API change to layoutdriver_io_operations free_layout

0001-SQUASHME-pnfs-submit-reinitialize-pnfs_layout_type.patch
0002-SQUASHME-pnfs-submit-file-layout-free_layout-init_on.patch

Tested

CONFIG_V4_I set:
Connectathon tests pass against GFS2/pNFS and pyNFS file layout servers. Both
return-on-close and not return-on-close tested.

CONFIG_V4_I not set:
NFSv4.0 mount passes Connectathon tests.

-->Andy

[PATCH 1/2] SQUASHME pnfs-submit: reinitialize pnfs_layout_type

[andros]

From: Andy Adamson <andros@netapp.com>

We do not free the pnfs_layout_type when the layout segment list is empty, but
we do remove the layout from the nfs_client cl_layouts list, and we also need
to re-initialize it. The next LAYOUTGET will set the layout values.

Note: API change: layoutdriver_io_operations free_layout now has a boolean
to indicate initiaize_only, do not free.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/pnfs.c             |   30 ++++++++++++++++++++++++++++--
 include/linux/nfs4_pnfs.h |    5 ++++-
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 6832237..d8887ef 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -337,7 +337,7 @@ put_layout_locked(struct pnfs_layout_type *lo)
 
 		dprintk("%s: freeing layout cache %p\n", __func__, lo);
 		WARN_ON(!list_empty(&lo->lo_layouts));
-		io_ops->free_layout(lo);
+		io_ops->free_layout(lo, false);
 		nfsi->layout = NULL;
 	}
 }
@@ -658,6 +658,31 @@ _pnfs_can_return_lseg(struct pnfs_layout_segment *lseg)
 	return atomic_read(&lseg->kref.refcount) == 1;
 }
 
+/*
+ * We don't free the pnfs_layout_type upon last LAYOUTRETURN, but we do
+ * un-initialize.  lo->segs is empty, and the layout has been removed from
+ * lo_layouts. Leave the lo->refcount un-touched.
+ *
+ * Called with inode->i_lock held.
+ *
+ * There still might be an outstanding async LAYOUTCOMMIT, but that's OK
+ * because the pnfs_layout_type is not referenced (exept for the refcount)
+ * by layoutcommit rpc_release.
+ *
+ * Call the layoutdriver free_layout with the initialze-only parameter set
+ * to true.
+ */
+static void nfs4_uninitialize_layout(struct pnfs_layout_type *lo)
+{
+	dprintk("--> %s lo->refcount %d\n", __func__, lo->refcount);
+	lo->roc_iomode = 0;
+	pnfs_set_layout_stateid(lo, &zero_stateid);
+	lo->pnfs_layout_state = 0;
+	lo->pnfs_write_begin_pos = 0;
+	lo->pnfs_write_end_pos = 0;
+	/* True means initialize, do not free the layout */
+	PNFS_LD_IO_OPS(lo)->free_layout(lo, true);
+}
 
 static void
 pnfs_free_layout(struct pnfs_layout_type *lo,
@@ -686,6 +711,7 @@ pnfs_free_layout(struct pnfs_layout_type *lo,
 		spin_lock(&clp->cl_lock);
 		list_del_init(&lo->lo_layouts);
 		spin_unlock(&clp->cl_lock);
+		nfs4_uninitialize_layout(lo);
 	}
 
 	dprintk("%s:Return\n", __func__);
@@ -959,7 +985,7 @@ nfs_lock_alloc_layout(struct inode *ino)
 		/* Reference the layout accross i_lock release and grab */
 		get_layout(NFS_I(ino)->layout);
 		spin_unlock(&ino->i_lock);
-		NFS_SERVER(ino)->pnfs_curr_ld->ld_io_ops->free_layout(new);
+		PNFS_LD_IO_OPS(NFS_I(ino)->layout)->free_layout(new, false);
 		spin_lock(&ino->i_lock);
 		put_layout_locked(NFS_I(ino)->layout);
 	}
diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
index 2c6ce4c..70e80cd 100644
--- a/include/linux/nfs4_pnfs.h
+++ b/include/linux/nfs4_pnfs.h
@@ -145,7 +145,10 @@ struct layoutdriver_io_operations {
 	 * inode specific layout structure.  Each subsequent layoutget operation results in
 	 * a set_layout call to set the opaque layout in the layout driver.*/
 	struct pnfs_layout_type * (*alloc_layout) (struct inode *inode);
-	void (*free_layout) (struct pnfs_layout_type *);
+
+	/* init_only true, means do not free the layout, just re-initialize */
+	void (*free_layout) (struct pnfs_layout_type *, bool init_only);
+
 	struct pnfs_layout_segment * (*alloc_lseg) (struct pnfs_layout_type *layoutid, struct nfs4_pnfs_layoutget_res *lgr);
 	void (*free_lseg) (struct pnfs_layout_segment *lseg);
 
-- 
1.6.6

[PATCH 2/2] SQUASHME pnfs-submit file layout free_layout init_only

[andros]

From: Andy Adamson <andros@netapp.com>

Implement the new free_layout layoutdriver io operation init_only boolean.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4filelayout.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index b49ccf4..e1b0d6c 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -314,9 +314,19 @@ filelayout_alloc_layout(struct inode *inode)
 /* Free a filelayout layout structure
  */
 static void
-filelayout_free_layout(struct pnfs_layout_type *lo)
+filelayout_free_layout(struct pnfs_layout_type *lo, bool init_only)
 {
-	dprintk("NFS_FILELAYOUT: freeing layout\n");
+	if (init_only) {
+		struct nfs4_filelayout *flp = FILE_LO(lo);
+
+		dprintk("%s re-intializing layout\n", __func__);
+		flp->uncommitted_write = 0;
+		flp->last_commit_size = 0;
+		flp->layout_id = 0;
+		flp->stripe_unit = 0;
+		return;
+	}
+	dprintk("%s freeing layout\n", __func__);
 	kfree(lo);
 }
 
-- 
1.6.6

Re: [PATCH 1/2] SQUASHME pnfs-submit: reinitialize pnfs_layout_type

[Benny Halevy]

On Jul. 09, 2010, 19:39 +0300, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> We do not free the pnfs_layout_type when the layout segment list is empty, but
> we do remove the layout from the nfs_client cl_layouts list, and we also need
> to re-initialize it. The next LAYOUTGET will set the layout values.
> 
> Note: API change: layoutdriver_io_operations free_layout now has a boolean
> to indicate initiaize_only, do not free.

I agree with Boaz that adding a new method (reinit_layout) will be much
better than overloading free_layout.

In any case, having the bool option stating exactly the opposite of
what the function name suggest is quite confusing :)

Benny

> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/pnfs.c             |   30 ++++++++++++++++++++++++++++--
>  include/linux/nfs4_pnfs.h |    5 ++++-
>  2 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index 6832237..d8887ef 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -337,7 +337,7 @@ put_layout_locked(struct pnfs_layout_type *lo)
>  
>  		dprintk("%s: freeing layout cache %p\n", __func__, lo);
>  		WARN_ON(!list_empty(&lo->lo_layouts));
> -		io_ops->free_layout(lo);
> +		io_ops->free_layout(lo, false);
>  		nfsi->layout = NULL;
>  	}
>  }
> @@ -658,6 +658,31 @@ _pnfs_can_return_lseg(struct pnfs_layout_segment *lseg)
>  	return atomic_read(&lseg->kref.refcount) == 1;
>  }
>  
> +/*
> + * We don't free the pnfs_layout_type upon last LAYOUTRETURN, but we do
> + * un-initialize.  lo->segs is empty, and the layout has been removed from
> + * lo_layouts. Leave the lo->refcount un-touched.
> + *
> + * Called with inode->i_lock held.
> + *
> + * There still might be an outstanding async LAYOUTCOMMIT, but that's OK
> + * because the pnfs_layout_type is not referenced (exept for the refcount)
> + * by layoutcommit rpc_release.
> + *
> + * Call the layoutdriver free_layout with the initialze-only parameter set
> + * to true.
> + */
> +static void nfs4_uninitialize_layout(struct pnfs_layout_type *lo)
> +{
> +	dprintk("--> %s lo->refcount %d\n", __func__, lo->refcount);
> +	lo->roc_iomode = 0;
> +	pnfs_set_layout_stateid(lo, &zero_stateid);
> +	lo->pnfs_layout_state = 0;
> +	lo->pnfs_write_begin_pos = 0;
> +	lo->pnfs_write_end_pos = 0;
> +	/* True means initialize, do not free the layout */
> +	PNFS_LD_IO_OPS(lo)->free_layout(lo, true);
> +}
>  
>  static void
>  pnfs_free_layout(struct pnfs_layout_type *lo,
> @@ -686,6 +711,7 @@ pnfs_free_layout(struct pnfs_layout_type *lo,
>  		spin_lock(&clp->cl_lock);
>  		list_del_init(&lo->lo_layouts);
>  		spin_unlock(&clp->cl_lock);
> +		nfs4_uninitialize_layout(lo);
>  	}
>  
>  	dprintk("%s:Return\n", __func__);
> @@ -959,7 +985,7 @@ nfs_lock_alloc_layout(struct inode *ino)
>  		/* Reference the layout accross i_lock release and grab */
>  		get_layout(NFS_I(ino)->layout);
>  		spin_unlock(&ino->i_lock);
> -		NFS_SERVER(ino)->pnfs_curr_ld->ld_io_ops->free_layout(new);
> +		PNFS_LD_IO_OPS(NFS_I(ino)->layout)->free_layout(new, false);
>  		spin_lock(&ino->i_lock);
>  		put_layout_locked(NFS_I(ino)->layout);
>  	}
> diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
> index 2c6ce4c..70e80cd 100644
> --- a/include/linux/nfs4_pnfs.h
> +++ b/include/linux/nfs4_pnfs.h
> @@ -145,7 +145,10 @@ struct layoutdriver_io_operations {
>  	 * inode specific layout structure.  Each subsequent layoutget operation results in
>  	 * a set_layout call to set the opaque layout in the layout driver.*/
>  	struct pnfs_layout_type * (*alloc_layout) (struct inode *inode);
> -	void (*free_layout) (struct pnfs_layout_type *);
> +
> +	/* init_only true, means do not free the layout, just re-initialize */
> +	void (*free_layout) (struct pnfs_layout_type *, bool init_only);
> +
>  	struct pnfs_layout_segment * (*alloc_lseg) (struct pnfs_layout_type *layoutid, struct nfs4_pnfs_layoutget_res *lgr);
>  	void (*free_lseg) (struct pnfs_layout_segment *lseg);
>  

Re: [PATCH 0/2] pnfs-submit Re-initialize pnfs_layout_type when segs list is empty

[Boaz Harrosh]

On 07/09/2010 07:39 PM, andros@netapp.com wrote:
> e keep the nfs_inode->layout when the segs list is empty, and we remove it
> from the nfs_client cl_layouts list, but we fail to reset the other fields.
> Re-initialize the layout (all except for the refcount) so that the next
> layoutget with potentially new deviceid.... sets the layout fields.
> 
> Note: API change to layoutdriver_io_operations free_layout
> 
> 0001-SQUASHME-pnfs-submit-reinitialize-pnfs_layout_type.patch
> 0002-SQUASHME-pnfs-submit-file-layout-free_layout-init_on.patch
> 
> Tested
> 
> CONFIG_V4_I set:
> Connectathon tests pass against GFS2/pNFS and pyNFS file layout servers. Both
> return-on-close and not return-on-close tested.
> 
> CONFIG_V4_I not set:
> NFSv4.0 mount passes Connectathon tests.
> 
> -->Andy
> 

Andy sorry to get on your case. But I hate it. It is an hack that takes
us back not forward. (Code less clean more obscure)

For starts don't save me a vector please. The two functions are unrelated
see your last patch 0002 there is no common code and a return in the middle
of an if() which should trigger an alarm for a bad API.

Second if at all, then the costume is to initialize on re-use not on end
of previous operation. (Clearer intentions, might not be reused, etc...)

3rd, if you'd follow my original proposal you'll see that this hack is not
needed.

If these patches fix an actual bug you've been hitting, could you just fix
the particular field in question that needs zeroing. And we can see how to
solve it better.

Thanks
Boaz

Re: [PATCH 0/2] pnfs-submit Re-initialize pnfs_layout_type when segs list is empty

[William A. (Andy) Adamson]

On Mon, Jul 12, 2010 at 11:48 AM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> On 07/09/2010 07:39 PM, andros@netapp.com wrote:
>> e keep the nfs_inode->layout when the segs list is empty, and we remove it
>> from the nfs_client cl_layouts list, but we fail to reset the other fields.
>> Re-initialize the layout (all except for the refcount) so that the next
>> layoutget with potentially new deviceid.... sets the layout fields.
>>
>> Note: API change to layoutdriver_io_operations free_layout
>>
>> 0001-SQUASHME-pnfs-submit-reinitialize-pnfs_layout_type.patch
>> 0002-SQUASHME-pnfs-submit-file-layout-free_layout-init_on.patch
>>
>> Tested
>>
>> CONFIG_V4_I set:
>> Connectathon tests pass against GFS2/pNFS and pyNFS file layout servers. Both
>> return-on-close and not return-on-close tested.
>>
>> CONFIG_V4_I not set:
>> NFSv4.0 mount passes Connectathon tests.
>>
>> -->Andy
>>
>
> Andy sorry to get on your case. But I hate it. It is an hack that takes
> us back not forward. (Code less clean more obscure)
>
> For starts don't save me a vector please. The two functions are unrelated
> see your last patch 0002 there is no common code and a return in the middle
> of an if() which should trigger an alarm for a bad API.

OK, that would be more clear.

>
> Second if at all, then the costume is to initialize on re-use not on end
> of previous operation. (Clearer intentions, might not be reused, etc...)

We need to zero the stateid at the return of the last layout segment.
Once we return the last segment, we signal the server that the layout
stateid is no longer in use and it can dispose of the layout state.

8.2.1. Stateid Types

      A stateid represents the set of all layouts held by a particular
      client for a particular filehandle with a given layout type.


12.5.3. Layout Stateid


   As with all other stateids, the layout stateid consists of a "seqid"
   and "other" field.  Once a layout stateid is established, the "other"
   field will stay constant unless the stateid is revoked or the client
   returns all layouts on the file and the server disposes of the
   stateid.

Once the stateid is no longer valid, the rest of the layout follows suit.


>
> 3rd, if you'd follow my original proposal you'll see that this hack is not
> needed.

If by you're original proposal you mean have the nfs_inode->layout
follow the life of the nfs_inode it is indeed needed!  You still need
to zero the stateid at the return of the last layout segment. You
still can't assume that any fields of the layout received from a
LAYOUTGET after you cleared the nfs_inode->layout->segs list will be
the same as the old layout returned. So, you need to re-initialize the
nfs_inode->layout.

>
> If these patches fix an actual bug you've been hitting, could you just fix
> the particular field in question that needs zeroing. And we can see how to
> solve it better.

The actual bug we are hitting is that we keep the stateid past the
last layout segment, and actually re-use it on a subsequent LAYOUTGET
(send_layout)

I should have added this to the patch comments.

-->Andy
>
> 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
>

Re: [PATCH 0/2] pnfs-submit Re-initialize pnfs_layout_type when segs list is empty

[Boaz Harrosh]

On 07/12/2010 07:36 PM, William A. (Andy) Adamson wrote:
> On Mon, Jul 12, 2010 at 11:48 AM, Boaz Harrosh <bharrosh@panasas.com> wrote:
>> On 07/09/2010 07:39 PM, andros@netapp.com wrote:
>>> e keep the nfs_inode->layout when the segs list is empty, and we remove it
>>> from the nfs_client cl_layouts list, but we fail to reset the other fields.
>>> Re-initialize the layout (all except for the refcount) so that the next
>>> layoutget with potentially new deviceid.... sets the layout fields.
>>>
>>> Note: API change to layoutdriver_io_operations free_layout
>>>
>>> 0001-SQUASHME-pnfs-submit-reinitialize-pnfs_layout_type.patch
>>> 0002-SQUASHME-pnfs-submit-file-layout-free_layout-init_on.patch
>>>
>>> Tested
>>>
>>> CONFIG_V4_I set:
>>> Connectathon tests pass against GFS2/pNFS and pyNFS file layout servers. Both
>>> return-on-close and not return-on-close tested.
>>>
>>> CONFIG_V4_I not set:
>>> NFSv4.0 mount passes Connectathon tests.
>>>
>>> -->Andy
>>>
>>
>> Andy sorry to get on your case. But I hate it. It is an hack that takes
>> us back not forward. (Code less clean more obscure)
>>
>> For starts don't save me a vector please. The two functions are unrelated
>> see your last patch 0002 there is no common code and a return in the middle
>> of an if() which should trigger an alarm for a bad API.
> 
> OK, that would be more clear.
> 
>>
>> Second if at all, then the costume is to initialize on re-use not on end
>> of previous operation. (Clearer intentions, might not be reused, etc...)
> 
> We need to zero the stateid at the return of the last layout segment.
> Once we return the last segment, we signal the server that the layout
> stateid is no longer in use and it can dispose of the layout state.
> 

Sure then we should have some _on_last_segment state somewhere, for example
when removing from client-list, right? Free the stateid there, and call it
free_stateid. and not call it reinit().

And is this a layoutdriver specific? It looks like generic business only.

> 8.2.1. Stateid Types
> 
>       A stateid represents the set of all layouts held by a particular
>       client for a particular filehandle with a given layout type.
> 
> 
> 12.5.3. Layout Stateid
> 
> 
>    As with all other stateids, the layout stateid consists of a "seqid"
>    and "other" field.  Once a layout stateid is established, the "other"
>    field will stay constant unless the stateid is revoked or the client
>    returns all layouts on the file and the server disposes of the
>    stateid.
> 
> Once the stateid is no longer valid, the rest of the layout follows suit.
> 
> 
>>
>> 3rd, if you'd follow my original proposal you'll see that this hack is not
>> needed.
> 
> If by you're original proposal you mean have the nfs_inode->layout
> follow the life of the nfs_inode it is indeed needed!  You still need
> to zero the stateid at the return of the last layout segment. You
> still can't assume that any fields of the layout received from a
> LAYOUTGET after you cleared the nfs_inode->layout->segs list will be
> the same as the old layout returned. So, you need to re-initialize the
> nfs_inode->layout.
> 

By now you should have a on_first_segment/on_last_segment states in code.
Generic part should clean sweep at on_first_segment state.

If you have a particular layout-driver specific initialization, LD can
check if segment is first in alloc_lseg and take care of itself. (Or
a flag could be supplied for the lazy)

>>
>> If these patches fix an actual bug you've been hitting, could you just fix
>> the particular field in question that needs zeroing. And we can see how to
>> solve it better.
> 
> The actual bug we are hitting is that we keep the stateid past the
> last layout segment, and actually re-use it on a subsequent LAYOUTGET
> (send_layout)
> 

As I said above: on_last_segment/remove-from-client-list can call a specific
free_lo_stateid, could be nice for readability.

> I should have added this to the patch comments.
> 
> -->Andy
>>
>> Thanks
>> Boaz

Boaz

Re: [PATCH 0/2] pnfs-submit Re-initialize pnfs_layout_type when segs list is empty

[William A. (Andy) Adamson]

On Mon, Jul 12, 2010 at 1:07 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> On 07/12/2010 07:36 PM, William A. (Andy) Adamson wrote:
>> On Mon, Jul 12, 2010 at 11:48 AM, Boaz Harrosh <bharrosh@panasas.com> wrote:
>>> On 07/09/2010 07:39 PM, andros@netapp.com wrote:
>>>> e keep the nfs_inode->layout when the segs list is empty, and we remove it
>>>> from the nfs_client cl_layouts list, but we fail to reset the other fields.
>>>> Re-initialize the layout (all except for the refcount) so that the next
>>>> layoutget with potentially new deviceid.... sets the layout fields.
>>>>
>>>> Note: API change to layoutdriver_io_operations free_layout
>>>>
>>>> 0001-SQUASHME-pnfs-submit-reinitialize-pnfs_layout_type.patch
>>>> 0002-SQUASHME-pnfs-submit-file-layout-free_layout-init_on.patch
>>>>
>>>> Tested
>>>>
>>>> CONFIG_V4_I set:
>>>> Connectathon tests pass against GFS2/pNFS and pyNFS file layout servers. Both
>>>> return-on-close and not return-on-close tested.
>>>>
>>>> CONFIG_V4_I not set:
>>>> NFSv4.0 mount passes Connectathon tests.
>>>>
>>>> -->Andy
>>>>
>>>
>>> Andy sorry to get on your case. But I hate it. It is an hack that takes
>>> us back not forward. (Code less clean more obscure)
>>>
>>> For starts don't save me a vector please. The two functions are unrelated
>>> see your last patch 0002 there is no common code and a return in the middle
>>> of an if() which should trigger an alarm for a bad API.
>>
>> OK, that would be more clear.
>>
>>>
>>> Second if at all, then the costume is to initialize on re-use not on end
>>> of previous operation. (Clearer intentions, might not be reused, etc...)
>>
>> We need to zero the stateid at the return of the last layout segment.
>> Once we return the last segment, we signal the server that the layout
>> stateid is no longer in use and it can dispose of the layout state.
>>
>
> Sure then we should have some _on_last_segment state somewhere, for example
> when removing from client-list, right? Free the stateid there, and call it
> free_stateid. and not call it reinit().

That is what this patch does -  zero the layout stateid when removed
from the client list. At this point, all information in the
pnfs_layout_type is old, so initialize.

>
> And is this a layoutdriver specific? It looks like generic business only.

The file layout driver zero's the stripe unit. I anticipate other
fields that would need to be zeroed.

>
>> 8.2.1. Stateid Types
>>
>>       A stateid represents the set of all layouts held by a particular
>>       client for a particular filehandle with a given layout type.
>>
>>
>> 12.5.3. Layout Stateid
>>
>>
>>    As with all other stateids, the layout stateid consists of a "seqid"
>>    and "other" field.  Once a layout stateid is established, the "other"
>>    field will stay constant unless the stateid is revoked or the client
>>    returns all layouts on the file and the server disposes of the
>>    stateid.
>>
>> Once the stateid is no longer valid, the rest of the layout follows suit.
>>
>>
>>>
>>> 3rd, if you'd follow my original proposal you'll see that this hack is not
>>> needed.
>>
>> If by you're original proposal you mean have the nfs_inode->layout
>> follow the life of the nfs_inode it is indeed needed!  You still need
>> to zero the stateid at the return of the last layout segment. You
>> still can't assume that any fields of the layout received from a
>> LAYOUTGET after you cleared the nfs_inode->layout->segs list will be
>> the same as the old layout returned. So, you need to re-initialize the
>> nfs_inode->layout.
>>

> By now you should have a on_first_segment/on_last_segment states in code.
> Generic part should clean sweep at on_first_segment state.

No need, already handled.

>
> If you have a particular layout-driver specific initialization, LD can
> check if segment is first in alloc_lseg and take care of itself. (Or
> a flag could be supplied for the lazy)

No need, already handled.

>
>>>
>>> If these patches fix an actual bug you've been hitting, could you just fix
>>> the particular field in question that needs zeroing. And we can see how to
>>> solve it better.
>>
>> The actual bug we are hitting is that we keep the stateid past the
>> last layout segment, and actually re-use it on a subsequent LAYOUTGET
>> (send_layout)
>>
>
> As I said above: on_last_segment/remove-from-client-list can call a specific
> free_lo_stateid, could be nice for readability.

It does: I call pnfs_set_layout_stateid() with the zero_stateid. That
is very clear.

I just don't see the need for two more states. We have a point in the
generic code where the layout segment list is empty. Therefore the
pnfs_layout_type information is stale. Simply initialize and all the
checks we have work.

-->Andy

>
>> I should have added this to the patch comments.
>>
>> -->Andy
>>>
>>> Thanks
>>> Boaz
>
> 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
>

Re: [PATCH 0/2] pnfs-submit Re-initialize pnfs_layout_type when segs list is empty

[Boaz Harrosh]

On 07/12/2010 09:07 PM, William A. (Andy) Adamson wrote:
> On Mon, Jul 12, 2010 at 1:07 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
>> >

>> > And is this a layoutdriver specific? It looks like generic business only.
> 
> The file layout driver zero's the stripe unit. I anticipate other
> fields that would need to be zeroed.
> 

The stripe_unit is per segment surly. It should be set on new segment
received. (Again the mix up between layout_type and layout_segment)
If stripe_unit also doubles for a flag that says, I have a layout.
Then zero it on free_lseg, when identified as last.

(Which also means that you are hard coding all-file-layout at heart
 of files-layout-driver)

>>
>> As I said above: on_last_segment/remove-from-client-list can call a specific
>> free_lo_stateid, could be nice for readability.
> 
> It does: I call pnfs_set_layout_stateid() with the zero_stateid. That
> is very clear.
> 
> I just don't see the need for two more states. 

You might miss-understood me. By state I meant: "a point in code where
it is identified that this is first/last segment" This already exists.
I did not mean like an NFS4 state machine.

I wish we had functions that said _on_first_seg, on_last_seg. But
that's just me.

> We have a point in the
> generic code where the layout segment list is empty. Therefore the
> pnfs_layout_type information is stale. Simply initialize and all the
> checks we have work.
> 

That is what I meant that I do not like. The initialize should be on
first_seg_insert, Not "last_seg_remove in case of re-insert". The only
check should be list-is-empty. Then on first insert, all should be initialized.

But I guess it's hard to talk about this. Submit the last fixes and I
can RFC my approach. I hope when you see it you'll be convinced.

Thanks
Boaz

> -->Andy

Re: [PATCH 0/2] pnfs-submit Re-initialize pnfs_layout_type when segs list is empty

[William A. (Andy) Adamson]

On Tue, Jul 13, 2010 at 5:02 AM, Boaz Harrosh <bharrosh@panasas.com> wr=
ote:
> On 07/12/2010 09:07 PM, William A. (Andy) Adamson wrote:
>> On Mon, Jul 12, 2010 at 1:07 PM, Boaz Harrosh <bharrosh@panasas.com>=
 wrote:
>>> >
>
>>> > And is this a layoutdriver specific? It looks like generic busine=
ss only.
>>
>> The file layout driver zero's the stripe unit. I anticipate other
>> fields that would need to be zeroed.
>>
>
> The stripe_unit is per segment surly. It should be set on new segment
> received. (Again the mix up between layout_type and layout_segment)
> If stripe_unit also doubles for a flag that says, I have a layout.
> Then zero it on free_lseg, when identified as last.


When we only have full file layouts, the only two possible layout
segments are full file segments with different iomodes, and so must
have an equal stripe unit.

I can init on free_lseg - agreed.

>
> (Which also means that you are hard coding all-file-layout at heart
> =A0of files-layout-driver)
>
>>>
>>> As I said above: on_last_segment/remove-from-client-list can call a=
 specific
>>> free_lo_stateid, could be nice for readability.
>>
>> It does: I call pnfs_set_layout_stateid() with the zero_stateid. Tha=
t
>> is very clear.
>>
>> I just don't see the need for two more states.
>
> You might miss-understood me. By state I meant: "a point in code wher=
e
> it is identified that this is first/last segment" This already exists=
=2E
> I did not mean like an NFS4 state machine.
>
> I wish we had functions that said _on_first_seg, on_last_seg. But
> that's just me.
>
>> We have a point in the
>> generic code where the layout segment list is empty. Therefore the
>> pnfs_layout_type information is stale. Simply initialize and all the
>> checks we have work.
>>
>
> That is what I meant that I do not like. The initialize should be on
> first_seg_insert, Not "last_seg_remove in case of re-insert". The onl=
y
> check should be list-is-empty.

No! The layout stateid needs to be zeroed as well.

>Then on first insert, all should be initialized.

I guess we just do things differently. I for one like to clean up the
dishes after the meal, not wait until before the next meal! But that's
just me.

-->Andy

>
> But I guess it's hard to talk about this. Submit the last fixes and I
> can RFC my approach. I hope when you see it you'll be convinced.

OK.

-->Andy

>
> Thanks
> Boaz
>
>> -->Andy
>