Skip to contentSkip to conversation

Does our Kernel PNFSD-Server supports recurring layout_get with open_state_id

PATCH In review Boaz Harrosh 2 participants 24 messages May 24, 2012 – May 31, 2012

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Boaz Harrosh

author
#0 lore raw

Benny hi

If I remember/understand correctly, there is a mode in the RFC errata about the layout forgetful-model and a client sending a layout_get with an open_state_id after he already had previous state (layouts) on the file.

As I understood this is an indication to the server that client has "forgotten" all it's layouts on a file, and Server can assume their return.

Is my understanding correct?

If Yes:
Did we implement the internal return of all layouts, if above open_state_id is encountered?
I thought we did but I can't find this code.

Currently, I always set ROC so there is no leak. But theoretically ROC does not have to be set. I'm doing some heavy lifting of layout_return, and I want to make sure I have not missed a spot.

If I'm correct that it is needed, and it's missing:
My suggestion for now is that we always set ROC, disregarding FS so not to leak layouts and therefor inode-refs, until such time that we implement it.

Thanks
Boaz

#1 lore raw
Quoted 7 lines · Boaz Harrosh

Benny hi

If I remember/understand correctly, there is a mode in the RFC errata about the layout forgetful-model and a client sending a layout_get with an open_state_id after he already had previous state (layouts) on the file.

Quoted 5 lines

As I understood this is an indication to the server that client has "forgotten" all it's layouts on a file, and Server can assume their return.

Is my understanding correct?

Yes

Quoted 5 lines

If Yes:
Did we implement the internal return of all layouts, if above open_state_id is encountered?
I thought we did but I can't find this code.

True. This is not implemented yet.

Quoted 8 lines

Currently, I always set ROC so there is no leak. But theoretically ROC does not have to be set. I'm doing some heavy lifting of layout_return, and I want to make sure I have not missed a spot.

If I'm correct that it is needed, and it's missing:
My suggestion for now is that we always set ROC, disregarding FS so not to leak layouts and therefor inode-refs, until such time that we implement it.

According to the new errata the server will have to simulate layout returns in the ROC case on last CLOSE if the (forgetful) client did not explicitly return the layout. This is not implemented either :-(

Benny

Quoted 7 lines

Thanks
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#2 lore raw
Quoted 29 lines · Benny Halevy
Quoted 7 lines · Boaz Harrosh

Benny hi

If I remember/understand correctly, there is a mode in the RFC errata about the layout forgetful-model and a client sending a layout_get with an open_state_id after he already had previous state (layouts) on the file.

Quoted 5 lines

As I understood this is an indication to the server that client has "forgotten" all it's layouts on a file, and Server can assume their return.

Is my understanding correct?

Yes

Quoted 5 lines

If Yes:
Did we implement the internal return of all layouts, if above open_state_id is encountered?
I thought we did but I can't find this code.

True. This is not implemented yet.

Quoted 5 lines

Currently, I always set ROC so there is no leak. But theoretically ROC does not have to be set. I'm doing some heavy lifting of layout_return, and I want to make sure I have not missed a spot.

Quoted 8 lines
Quoted 3 lines

If I'm correct that it is needed, and it's missing:
My suggestion for now is that we always set ROC, disregarding FS so not to leak layouts and therefor inode-refs, until such time that we implement it.

According to the new errata the server will have to simulate layout returns in the ROC case on last CLOSE if the (forgetful) client did not explicitly return the layout. This is not implemented either :-(

Yes Benny it is implemented. I implemented it and you fix a bug I had. See:

	nfs4pnfsd.c::pnfsd_roc(...)

and it's call site. It is called on last call and all is working well (except some locking bugs I'll send a fix to later). Otherwise both exofs and DF would have crapped out for sure.

With ROC set all work well. This is why I say we should set it for now to make sure we do not have the above bug.

Thanks
Boaz

Quoted 9 lines

Benny

Quoted 7 lines

Thanks
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#3 lore raw
Quoted 1 line · Boaz Harrosh

and it's call site. It is called on last call and all is working well (except some locking

I meant last close not last call

Boaz

#4 lore raw
In reply to Boaz Harrosh
Quoted 41 lines · Boaz Harrosh
Quoted 29 lines · Benny Halevy
Quoted 7 lines · Boaz Harrosh

Benny hi

If I remember/understand correctly, there is a mode in the RFC errata about the layout forgetful-model and a client sending a layout_get with an open_state_id after he already had previous state (layouts) on the file.

Quoted 5 lines

As I understood this is an indication to the server that client has "forgotten" all it's layouts on a file, and Server can assume their return.

Is my understanding correct?

Yes

Quoted 5 lines

If Yes:
Did we implement the internal return of all layouts, if above open_state_id is encountered?
I thought we did but I can't find this code.

True. This is not implemented yet.

Quoted 5 lines

Currently, I always set ROC so there is no leak. But theoretically ROC does not have to be set. I'm doing some heavy lifting of layout_return, and I want to make sure I have not missed a spot.

Quoted 8 lines
Quoted 3 lines

If I'm correct that it is needed, and it's missing:
My suggestion for now is that we always set ROC, disregarding FS so not to leak layouts and therefor inode-refs, until such time that we implement it.

According to the new errata the server will have to simulate layout returns in the ROC case on last CLOSE if the (forgetful) client did not explicitly return the layout. This is not implemented either :-(

Quoted 4 lines

Yes Benny it is implemented. I implemented it and you fix a bug I had. See:

	nfs4pnfsd.c::pnfsd_roc(...)

and it's call site. It is called on last call and all is working well (except some locking bugs I'll send a fix to later). Otherwise both exofs and DF would have crapped out for sure.

Duh, you're right of course.

Quoted 3 lines

With ROC set all work well. This is why I say we should set it for now to make sure we do not have the above bug.

Yup, see patchset in reply.

Thanks!

Benny

Quoted 19 lines

Thanks
Boaz

Quoted 9 lines

Benny

Quoted 7 lines

Thanks
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw

PATCH 1/5 pnfsd: make return_on_close state layout-global for the file/client

As per RFC5661 errata #3226
http://www.ietf.org/mail-archive/web/nfsv4/current/msg10965.html
Once the server returns the return_on_close flag set, all the layout for that client will be implicitly returned on last close.

Reported-by Boaz Harrosh <[email protected]>
Signed-off-by Benny Halevy <[email protected]>
---
 fs/nfsd/nfs4pnfsd.c |   19 ++++++++++++++-----
 fs/nfsd/pnfsd.h     |    2 +-
 2 files changed, 15 insertions(+), 6 deletions(-)
fs/nfsd/nfs4pnfsd.c +14 -5
index 9f94cd0..a95e96e 100644
@@ -161,6 +161,7 @@ void pnfs_clear_device_notify(struct nfs4_client *clp)
 	spin_lock(&layout_lock);
 	list_add(&new->ls_perfile, &fp->fi_layout_states);
 	spin_unlock(&layout_lock);
+	new->ls_roc = false;
 	return new;
 }
 

@@ -286,6 +287,15 @@ static void update_layout_stateid(struct nfs4_layout_state *ls, stateid_t *sid)
 	spin_unlock(&layout_lock);
 }
 
+static void update_layout_roc(struct nfs4_layout_state *ls, bool roc)
+{
+	if (roc) {
+		ls->ls_roc = true;
+		dprintk("%s: Marked return_on_close on layoutstate %p\n",
+			__func__, ls);
+	}
+}
+
 static void
 init_layout(struct nfs4_layout *lp,
 	    struct nfs4_layout_state *ls,

@@ -293,8 +303,7 @@ static void update_layout_stateid(struct nfs4_layout_state *ls, stateid_t *sid)
 	    struct nfs4_client *clp,
 	    struct svc_fh *current_fh,
 	    struct nfsd4_layout_seg *seg,
-	    stateid_t *stateid,
-	    bool roc)
+	    stateid_t *stateid)
 {
 	dprintk("pNFS %s: lp %p ls %p clp %p fp %p ino %p\n", __func__,
 		lp, ls, clp, fp, fp->fi_inode);

@@ -305,7 +314,6 @@ static void update_layout_stateid(struct nfs4_layout_state *ls, stateid_t *sid)
 	memcpy(&lp->lo_seg, seg, sizeof(lp->lo_seg));
 	get_layout_state(ls);		/* put on destroy_layout */
 	lp->lo_state = ls;
-	lp->lo_roc = roc;
 	update_layout_stateid(ls, stateid);
 	list_add_tail(&lp->lo_perclnt, &clp->cl_layouts);
 	list_add_tail(&lp->lo_perfile, &fp->fi_layouts);

@@ -811,6 +819,7 @@ struct super_block *
 
 	lgp->lg_seg = res.lg_seg;
 	lgp->lg_roc = res.lg_return_on_close;
+	update_layout_roc(ls, res.lg_return_on_close);
 
 	/* SUCCESS!
 	 * Can the new layout be merged into an existing one?

@@ -820,7 +829,7 @@ struct super_block *
 		goto out_freelayout;
 
 	/* Can't merge, so let's initialize this new layout */
-	init_layout(lp, ls, fp, clp, lgp->lg_fhp, &res.lg_seg, &lgp->lg_sid, res.lg_return_on_close);
+	init_layout(lp, ls, fp, clp, lgp->lg_fhp, &res.lg_seg, &lgp->lg_sid);
 out_unlock:
 	if (ls)
 		put_layout_state(ls);

@@ -1225,7 +1234,7 @@ void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp)
 		bool empty;
 
 		/* Check for a match */
-		if (!lo->lo_roc || lo->lo_client != clp)
+		if (!lo->lo_state->ls_roc || lo->lo_client != clp)
 			continue;
 
 		/* Return the layout */
fs/nfsd/pnfsd.h +1 -1
index f0862fb..e960fd3 100644
@@ -45,6 +45,7 @@ struct nfs4_layout_state {
 	struct kref		ls_ref;
 	struct nfs4_stid	ls_stid;
 	struct list_head	ls_perfile;
+	bool			ls_roc;
 };
 
 /* outstanding layout */

@@ -55,7 +56,6 @@ struct nfs4_layout {
 	struct nfs4_client		*lo_client;
 	struct nfs4_layout_state	*lo_state;
 	struct nfsd4_layout_seg		lo_seg;
-	bool				lo_roc;
 };
 
 struct pnfs_inval_state {
Signature · 1 line
1.7.6.5
author
#6 lore raw

Re: [PATCH 1/5] pnfsd: make return_on_close state layout-global for the file/client

Quoted 5 lines · Benny Halevy

As per RFC5661 errata #3226
http://www.ietf.org/mail-archive/web/nfsv4/current/msg10965.html
Once the server returns the return_on_close flag set, all the layout for that client will be implicitly returned on last close.

This one I'll keep though It will conflict with my patches.

I will incorporate whats relevant.

Quoted 99 lines
Reported-by Boaz Harrosh <[email protected]>
Signed-off-by Benny Halevy <[email protected]>
---
 fs/nfsd/nfs4pnfsd.c |   19 ++++++++++++++-----
 fs/nfsd/pnfsd.h     |    2 +-
 2 files changed, 15 insertions(+), 6 deletions(-)
fs/nfsd/nfs4pnfsd.c +14 -5
index 9f94cd0..a95e96e 100644
@@ -161,6 +161,7 @@ void pnfs_clear_device_notify(struct nfs4_client *clp)
 	spin_lock(&layout_lock);
 	list_add(&new->ls_perfile, &fp->fi_layout_states);
 	spin_unlock(&layout_lock);
+	new->ls_roc = false;
 	return new;
 }
 

@@ -286,6 +287,15 @@ static void update_layout_stateid(struct nfs4_layout_state *ls, stateid_t *sid)
 	spin_unlock(&layout_lock);
 }
 
+static void update_layout_roc(struct nfs4_layout_state *ls, bool roc)
+{
+	if (roc) {
+		ls->ls_roc = true;
+		dprintk("%s: Marked return_on_close on layoutstate %p\n",
+			__func__, ls);
+	}
+}
+
 static void
 init_layout(struct nfs4_layout *lp,
 	    struct nfs4_layout_state *ls,

@@ -293,8 +303,7 @@ static void update_layout_stateid(struct nfs4_layout_state *ls, stateid_t *sid)
 	    struct nfs4_client *clp,
 	    struct svc_fh *current_fh,
 	    struct nfsd4_layout_seg *seg,
-	    stateid_t *stateid,
-	    bool roc)
+	    stateid_t *stateid)
 {
 	dprintk("pNFS %s: lp %p ls %p clp %p fp %p ino %p\n", __func__,
 		lp, ls, clp, fp, fp->fi_inode);

@@ -305,7 +314,6 @@ static void update_layout_stateid(struct nfs4_layout_state *ls, stateid_t *sid)
 	memcpy(&lp->lo_seg, seg, sizeof(lp->lo_seg));
 	get_layout_state(ls);		/* put on destroy_layout */
 	lp->lo_state = ls;
-	lp->lo_roc = roc;
 	update_layout_stateid(ls, stateid);
 	list_add_tail(&lp->lo_perclnt, &clp->cl_layouts);
 	list_add_tail(&lp->lo_perfile, &fp->fi_layouts);

@@ -811,6 +819,7 @@ struct super_block *
 
 	lgp->lg_seg = res.lg_seg;
 	lgp->lg_roc = res.lg_return_on_close;
+	update_layout_roc(ls, res.lg_return_on_close);
 
 	/* SUCCESS!
 	 * Can the new layout be merged into an existing one?

@@ -820,7 +829,7 @@ struct super_block *
 		goto out_freelayout;
 
 	/* Can't merge, so let's initialize this new layout */
-	init_layout(lp, ls, fp, clp, lgp->lg_fhp, &res.lg_seg, &lgp->lg_sid, res.lg_return_on_close);
+	init_layout(lp, ls, fp, clp, lgp->lg_fhp, &res.lg_seg, &lgp->lg_sid);
 out_unlock:
 	if (ls)
 		put_layout_state(ls);

@@ -1225,7 +1234,7 @@ void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp)
 		bool empty;
 
 		/* Check for a match */
-		if (!lo->lo_roc || lo->lo_client != clp)
+		if (!lo->lo_state->ls_roc || lo->lo_client != clp)
 			continue;
 
 		/* Return the layout */
fs/nfsd/pnfsd.h +1 -1
index f0862fb..e960fd3 100644
@@ -45,6 +45,7 @@ struct nfs4_layout_state {
 	struct kref		ls_ref;
 	struct nfs4_stid	ls_stid;
 	struct list_head	ls_perfile;
+	bool			ls_roc;
 };
 
 /* outstanding layout */

@@ -55,7 +56,6 @@ struct nfs4_layout {
 	struct nfs4_client		*lo_client;
 	struct nfs4_layout_state	*lo_state;
 	struct nfsd4_layout_seg		lo_seg;
-	bool				lo_roc;
 };
 
 struct pnfs_inval_state {
#7 lore raw

PATCH 2/5 SQUASHME: pnfsd: use LR_FLAG_INTERN for pnfsd_roc implicit layout return

In reply to Benny Halevy

The client is not expired in the case, but rather we internally simulate a layout return based on return_on_close.

Signed-off-by Benny Halevy <[email protected]>
---
 fs/nfsd/nfs4pnfsd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/nfsd/nfs4pnfsd.c +1 -1
index a95e96e..cfaac56 100644
@@ -1246,7 +1246,7 @@ void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp)
 
 		empty = list_empty(&fp->fi_layouts);
 		fs_layout_return(fp->fi_inode->i_sb, fp->fi_inode, &lr,
-				 LR_FLAG_EXPIRE,
+				 LR_FLAG_INTERN,
 				 empty ? PNFS_LAST_LAYOUT_NO_RECALLS : NULL);
 	}
 	spin_unlock(&layout_lock);
Signature · 1 line
1.7.6.5
#8 lore raw

PATCH 3/5 pnfsd: add debug printouts to pnfsd_roc

In reply to Benny Halevy
Signed-off-by Benny Halevy <[email protected]>
---
 fs/nfsd/nfs4pnfsd.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
fs/nfsd/nfs4pnfsd.c +6 -0
index cfaac56..0a8d5b5 100644
@@ -1227,7 +1227,9 @@ int nfs4_pnfs_return_layout(struct super_block *sb, struct svc_fh *current_fh,
 void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp)
 {
 	struct nfs4_layout *lo, *nextlp;
+	bool found = false;
 
+	dprintk("%s: fp=%p clp=%p", __func__, fp, clp);
 	spin_lock(&layout_lock);
 	list_for_each_entry_safe (lo, nextlp, &fp->fi_layouts, lo_perfile) {
 		struct nfsd4_pnfs_layoutreturn lr;

@@ -1245,11 +1247,15 @@ void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp)
 		destroy_layout(lo); /* do not access lp after this */
 
 		empty = list_empty(&fp->fi_layouts);
+		found = true;
+		dprintk("%s: fp=%p clp=%p: return on close", __func__, fp, clp);
 		fs_layout_return(fp->fi_inode->i_sb, fp->fi_inode, &lr,
 				 LR_FLAG_INTERN,
 				 empty ? PNFS_LAST_LAYOUT_NO_RECALLS : NULL);
 	}
 	spin_unlock(&layout_lock);
+	if (!found)
+		dprintk("%s: no layout found", __func__);
 }
 
 void pnfs_expire_client(struct nfs4_client *clp)
Signature · 1 line
1.7.6.5
author
#9 lore raw

Re: [PATCH 3/5] pnfsd: add debug printouts to pnfsd_roc

Quoted 1 line · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>

Benny I disagree with this patch.

Not specifically with the print but with the !found print.

pnfsd_roc is always called layouts or not. So these !found print are bogus.

Quoted 19 lines
---
 fs/nfsd/nfs4pnfsd.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
fs/nfsd/nfs4pnfsd.c +2 -0
index cfaac56..0a8d5b5 100644
@@ -1227,7 +1227,9 @@ int nfs4_pnfs_return_layout(struct super_block *sb, struct svc_fh *current_fh,
 void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp)
 {
 	struct nfs4_layout *lo, *nextlp;
+	bool found = false;
 
+	dprintk("%s: fp=%p clp=%p", __func__, fp, clp);
 	spin_lock(&layout_lock);
 	list_for_each_entry_safe (lo, nextlp, &fp->fi_layouts, lo_perfile) {
 		struct nfsd4_pnfs_layoutreturn lr;

fs/nfsd/nfs4pnfsd.c void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp) · line 1254

Quoted 8 lines
fs/nfsd/nfs4pnfsd.c +2 -0
@@ -1245,11 +1247,15 @@ void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp)
 		destroy_layout(lo); /* do not access lp after this */
 
 		empty = list_empty(&fp->fi_layouts);
+		found = true;
+		dprintk("%s: fp=%p clp=%p: return on close", __func__, fp, clp);
 		fs_layout_return(fp->fi_inode->i_sb, fp->fi_inode, &lr,
 				 LR_FLAG_INTERN,
 				 empty ? PNFS_LAST_LAYOUT_NO_RECALLS : NULL);

fs_layout_return already prints for each call. So we should only add one global print so to know we came from ROC

Please don't fix it. I have this covered in my patches. In fact this function is totally changed.

fs/nfsd/nfs4pnfsd.c void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp) · line 1258

Quoted 4 lines
fs/nfsd/nfs4pnfsd.c +2 -0
 	}
 	spin_unlock(&layout_lock);
+	if (!found)
+		dprintk("%s: no layout found", __func__);

Again please no prints here. pnfsd_roc is always called unconditionally from last close.
(And found should be conditional on SUNRPC_DEBUG if is left)

fs/nfsd/nfs4pnfsd.c void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp) · line 1261

Quoted 3 lines
fs/nfsd/nfs4pnfsd.c +0 -0
 }
 
 void pnfs_expire_client(struct nfs4_client *clp)

Thanks
Boaz

#10 lore raw

Re: [PATCH 3/5] pnfsd: add debug printouts to pnfsd_roc

Quoted 44 lines · Boaz Harrosh
Quoted 1 line · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>

Benny I disagree with this patch.

Not specifically with the print but with the !found print.

pnfsd_roc is always called layouts or not. So these !found print are bogus.

Quoted 19 lines
---
 fs/nfsd/nfs4pnfsd.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
fs/nfsd/nfs4pnfsd.c +2 -0
index cfaac56..0a8d5b5 100644
@@ -1227,7 +1227,9 @@ int nfs4_pnfs_return_layout(struct super_block *sb, struct svc_fh *current_fh,
 void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp)
 {
 	struct nfs4_layout *lo, *nextlp;
+	bool found = false;
 
+	dprintk("%s: fp=%p clp=%p", __func__, fp, clp);
 	spin_lock(&layout_lock);
 	list_for_each_entry_safe (lo, nextlp, &fp->fi_layouts, lo_perfile) {
 		struct nfsd4_pnfs_layoutreturn lr;

fs/nfsd/nfs4pnfsd.c void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp) · line 1254

Quoted 8 lines
fs/nfsd/nfs4pnfsd.c +2 -0
@@ -1245,11 +1247,15 @@ void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp)
 		destroy_layout(lo); /* do not access lp after this */
 
 		empty = list_empty(&fp->fi_layouts);
+		found = true;
+		dprintk("%s: fp=%p clp=%p: return on close", __func__, fp, clp);
 		fs_layout_return(fp->fi_inode->i_sb, fp->fi_inode, &lr,
 				 LR_FLAG_INTERN,
 				 empty ? PNFS_LAST_LAYOUT_NO_RECALLS : NULL);

fs_layout_return already prints for each call. So we should only add one global print so to know we came from ROC

Quoted 3 lines

Please don't fix it. I have this covered in my patches. In fact this function is totally changed.

OK.

Quoted 21 lines

fs/nfsd/nfs4pnfsd.c void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp) · line 1258

Quoted 4 lines
fs/nfsd/nfs4pnfsd.c +2 -0
 	}
 	spin_unlock(&layout_lock);
+	if (!found)
+		dprintk("%s: no layout found", __func__);

Again please no prints here. pnfsd_roc is always called unconditionally from last close.
(And found should be conditional on SUNRPC_DEBUG if is left)

fs/nfsd/nfs4pnfsd.c void pnfsd_roc(struct nfs4_client *clp, struct nfs4_file *fp) · line 1261

Quoted 3 lines
fs/nfsd/nfs4pnfsd.c +0 -0
 }
 
 void pnfs_expire_client(struct nfs4_client *clp)

Thanks
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#11 lore raw

PATCH 4/5 pnfsd-lexp: return_on_close config option

In reply to Benny Halevy
Signed-off-by Benny Halevy <[email protected]>
---
 fs/nfsd/Kconfig      |    7 +++++++
 fs/nfsd/pnfsd_lexp.c |    5 +++++
 2 files changed, 12 insertions(+), 0 deletions(-)
fs/nfsd/Kconfig +7 -0
index 9d4d79b..f9b3426 100644
@@ -132,3 +132,10 @@ config PNFSD_LEXP_LAYOUT_SEGMENT_SIZE
 	  Set simulated layout segment size.
 
 	  If unsure, say N.
+
+config PNFSD_LEXP_RETURN_ON_CLOSE
+	bool "Reply to LAYOUTGET with return_on_close set to true"
+	depends on PNFSD_LOCAL_EXPORT
+	default false
+	help
+	  Set return_on_close response flag.
fs/nfsd/pnfsd_lexp.c +5 -0
index 30d9757..33a724c 100644
@@ -153,6 +153,11 @@ static int get_stripe_unit(int blocksize)
 	res->lg_seg.offset = 0;
 	res->lg_seg.length = NFS4_MAX_UINT64;
 #endif  /* CONFIG_PNFSD_LEXP_LAYOUT_SEGMENTS */
+#ifdef     CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE
+	res->lg_return_on_close = true;
+#else   /* CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE */
+	res->lg_return_on_close = false;
+#endif  /* CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE */
 
 	layout = kzalloc(sizeof(*layout), GFP_KERNEL);
 	if (layout == NULL) {
Signature · 1 line
1.7.6.5
author
#12 lore raw

Re: [PATCH 4/5] pnfsd-lexp: return_on_close config option

Quoted 26 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>
---
 fs/nfsd/Kconfig      |    7 +++++++
 fs/nfsd/pnfsd_lexp.c |    5 +++++
 2 files changed, 12 insertions(+), 0 deletions(-)
fs/nfsd/Kconfig +7 -0
index 9d4d79b..f9b3426 100644
@@ -132,3 +132,10 @@ config PNFSD_LEXP_LAYOUT_SEGMENT_SIZE
 	  Set simulated layout segment size.
 
 	  If unsure, say N.
+
+config PNFSD_LEXP_RETURN_ON_CLOSE
+	bool "Reply to LAYOUTGET with return_on_close set to true"
+	depends on PNFSD_LOCAL_EXPORT
+	default false
+	help
+	  Set return_on_close response flag.
fs/nfsd/pnfsd_lexp.c +0 -0
index 30d9757..33a724c 100644

fs/nfsd/pnfsd_lexp.c static int get_stripe_unit(int blocksize) · line 159

Quoted 7 lines
fs/nfsd/pnfsd_lexp.c +4 -0
@@ -153,6 +153,11 @@ static int get_stripe_unit(int blocksize)
 	res->lg_seg.offset = 0;
 	res->lg_seg.length = NFS4_MAX_UINT64;
 #endif  /* CONFIG_PNFSD_LEXP_LAYOUT_SEGMENTS */
+#ifdef     CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE
+	res->lg_return_on_close = true;
+#else   /* CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE */
+	res->lg_return_on_close = false;

Back to my original question. With current code this case is a BUG. It will leak layouts and therefore file-reference in case of a layout-get with open-state ID.

So I suggest hardcoding to true until such time that we actually can support it

Boaz

Quoted 4 lines
fs/nfsd/pnfsd_lexp.c +1 -0
+#endif  /* CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE */
 
 	layout = kzalloc(sizeof(*layout), GFP_KERNEL);
 	if (layout == NULL) {
#13 lore raw

Re: [PATCH 4/5] pnfsd-lexp: return_on_close config option

Quoted 41 lines · Boaz Harrosh
Quoted 26 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>
---
 fs/nfsd/Kconfig      |    7 +++++++
 fs/nfsd/pnfsd_lexp.c |    5 +++++
 2 files changed, 12 insertions(+), 0 deletions(-)
fs/nfsd/Kconfig +7 -0
index 9d4d79b..f9b3426 100644
@@ -132,3 +132,10 @@ config PNFSD_LEXP_LAYOUT_SEGMENT_SIZE
 	  Set simulated layout segment size.
 
 	  If unsure, say N.
+
+config PNFSD_LEXP_RETURN_ON_CLOSE
+	bool "Reply to LAYOUTGET with return_on_close set to true"
+	depends on PNFSD_LOCAL_EXPORT
+	default false
+	help
+	  Set return_on_close response flag.
fs/nfsd/pnfsd_lexp.c +0 -0
index 30d9757..33a724c 100644

fs/nfsd/pnfsd_lexp.c static int get_stripe_unit(int blocksize) · line 159

Quoted 7 lines
fs/nfsd/pnfsd_lexp.c +4 -0
@@ -153,6 +153,11 @@ static int get_stripe_unit(int blocksize)
 	res->lg_seg.offset = 0;
 	res->lg_seg.length = NFS4_MAX_UINT64;
 #endif  /* CONFIG_PNFSD_LEXP_LAYOUT_SEGMENTS */
+#ifdef     CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE
+	res->lg_return_on_close = true;
+#else   /* CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE */
+	res->lg_return_on_close = false;

Back to my original question. With current code this case is a BUG. It will leak layouts and therefore file-reference in case of a layout-get with open-state ID.

Quoted 2 lines

So I suggest hardcoding to true until such time that we actually can support it

so how will you debug the leak case?
pnfsd-lexp is for testing, and the default is on.

Benny

Quoted 13 lines

Boaz

fs/nfsd/pnfsd_lexp.c static int get_stripe_unit(int blocksize) · line 163

Quoted 4 lines
fs/nfsd/pnfsd_lexp.c +1 -0
+#endif  /* CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE */
 
 	layout = kzalloc(sizeof(*layout), GFP_KERNEL);
 	if (layout == NULL) {

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#14 lore raw

Re: [PATCH 4/5] pnfsd-lexp: return_on_close config option

Quoted 42 lines · Benny Halevy
Quoted 41 lines · Boaz Harrosh
Quoted 26 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>
---
 fs/nfsd/Kconfig      |    7 +++++++
 fs/nfsd/pnfsd_lexp.c |    5 +++++
 2 files changed, 12 insertions(+), 0 deletions(-)
fs/nfsd/Kconfig +7 -0
index 9d4d79b..f9b3426 100644
@@ -132,3 +132,10 @@ config PNFSD_LEXP_LAYOUT_SEGMENT_SIZE
 	  Set simulated layout segment size.
 
 	  If unsure, say N.
+
+config PNFSD_LEXP_RETURN_ON_CLOSE
+	bool "Reply to LAYOUTGET with return_on_close set to true"
+	depends on PNFSD_LOCAL_EXPORT
+	default false
+	help
+	  Set return_on_close response flag.
fs/nfsd/pnfsd_lexp.c +0 -0
index 30d9757..33a724c 100644

fs/nfsd/pnfsd_lexp.c static int get_stripe_unit(int blocksize) · line 159

Quoted 7 lines
fs/nfsd/pnfsd_lexp.c +4 -0
@@ -153,6 +153,11 @@ static int get_stripe_unit(int blocksize)
 	res->lg_seg.offset = 0;
 	res->lg_seg.length = NFS4_MAX_UINT64;
 #endif  /* CONFIG_PNFSD_LEXP_LAYOUT_SEGMENTS */
+#ifdef     CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE
+	res->lg_return_on_close = true;
+#else   /* CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE */
+	res->lg_return_on_close = false;

Back to my original question. With current code this case is a BUG. It will leak layouts and therefore file-reference in case of a layout-get with open-state ID.

Quoted 5 lines
Quoted 2 lines

So I suggest hardcoding to true until such time that we actually can support it

so how will you debug the leak case?
pnfsd-lexp is for testing, and the default is on.

oops, the default is actually off in this patch.
I do need to code what I wish :)

Benny

Quoted 16 lines

Benny

Quoted 13 lines

Boaz

fs/nfsd/pnfsd_lexp.c static int get_stripe_unit(int blocksize) · line 163

Quoted 4 lines
fs/nfsd/pnfsd_lexp.c +1 -0
+#endif  /* CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE */
 
 	layout = kzalloc(sizeof(*layout), GFP_KERNEL);
 	if (layout == NULL) {

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#15 lore raw

Re: [PATCH 4/5] pnfsd-lexp: return_on_close config option

Quoted 43 lines · Benny Halevy
Quoted 42 lines · Benny Halevy
Quoted 41 lines · Boaz Harrosh
Quoted 26 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>
---
 fs/nfsd/Kconfig      |    7 +++++++
 fs/nfsd/pnfsd_lexp.c |    5 +++++
 2 files changed, 12 insertions(+), 0 deletions(-)
fs/nfsd/Kconfig +7 -0
index 9d4d79b..f9b3426 100644
@@ -132,3 +132,10 @@ config PNFSD_LEXP_LAYOUT_SEGMENT_SIZE
 	  Set simulated layout segment size.
 
 	  If unsure, say N.
+
+config PNFSD_LEXP_RETURN_ON_CLOSE
+	bool "Reply to LAYOUTGET with return_on_close set to true"
+	depends on PNFSD_LOCAL_EXPORT
+	default false
+	help
+	  Set return_on_close response flag.
fs/nfsd/pnfsd_lexp.c +0 -0
index 30d9757..33a724c 100644

fs/nfsd/pnfsd_lexp.c static int get_stripe_unit(int blocksize) · line 159

Quoted 7 lines
fs/nfsd/pnfsd_lexp.c +4 -0
@@ -153,6 +153,11 @@ static int get_stripe_unit(int blocksize)
 	res->lg_seg.offset = 0;
 	res->lg_seg.length = NFS4_MAX_UINT64;
 #endif  /* CONFIG_PNFSD_LEXP_LAYOUT_SEGMENTS */
+#ifdef     CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE
+	res->lg_return_on_close = true;
+#else   /* CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE */
+	res->lg_return_on_close = false;

Back to my original question. With current code this case is a BUG. It will leak layouts and therefore file-reference in case of a layout-get with open-state ID.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

So I suggest hardcoding to true until such time that we actually can support it

so how will you debug the leak case?
pnfsd-lexp is for testing, and the default is on.

oops, the default is actually off in this patch.
I do need to code what I wish :)

Fixed.

Quoted 19 lines

Benny

Quoted 16 lines

Benny

Quoted 13 lines

Boaz

fs/nfsd/pnfsd_lexp.c static int get_stripe_unit(int blocksize) · line 163

Quoted 4 lines
fs/nfsd/pnfsd_lexp.c +1 -0
+#endif  /* CONFIG_PNFSD_LEXP_RETURN_ON_CLOSE */
 
 	layout = kzalloc(sizeof(*layout), GFP_KERNEL);
 	if (layout == NULL) {

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#16 lore raw

Re: [PATCH 4/5] pnfsd-lexp: return_on_close config option

In reply to Benny Halevy
Quoted 7 lines · Benny Halevy

On 2012-05-28 19:52, Boaz Harrosh wrote:

so how will you debug the leak case?
pnfsd-lexp is for testing, and the default is on.

Benny

Good point I agree

Boaz

#17 lore raw

PATCH 5/5 SQUASHME: pnfsd: lrs_present is false by default

In reply to Benny Halevy

Set to true only on error free path.

Signed-off-by Benny Halevy <[email protected]>
---
 fs/nfsd/nfs4pnfsd.c |    1 +
 fs/nfsd/nfs4proc.c  |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)
fs/nfsd/nfs4pnfsd.c +1 -0
index 0a8d5b5..11bccdf 100644
@@ -892,6 +892,7 @@ struct super_block *
 	struct nfs4_layout *lp, *nextlp;
 
 	dprintk("%s: clp %p fp %p\n", __func__, clp, fp);
+	lrp->lrs_present = 1;
 	spin_lock(&layout_lock);
 	list_for_each_entry_safe (lp, nextlp, &fp->fi_layouts, lo_perfile) {
 		dprintk("%s: lp %p client %p,%p lo_type %x,%x iomode %d,%d\n",
fs/nfsd/nfs4proc.c +1 -1
index 6328300..e76111c 100644
@@ -1299,7 +1299,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
 
 	/* Set clientid from sessionid */
 	copy_clientid((clientid_t *)&lrp->args.lr_seg.clientid, cstate->session);
-	lrp->lrs_present = (lrp->args.lr_return_type == RETURN_FILE);
+	lrp->lrs_present = 0;
 	status = nfs4_pnfs_return_layout(sb, current_fh, lrp);
 out:
 	dprintk("pNFS %s: status %d return_type 0x%x lrs_present %d\n",
Signature · 1 line
1.7.6.5
author
#18 lore raw
In reply to Benny Halevy
Quoted 2 lines · Benny Halevy

Yup, see patchset in reply.

That I wish you wouldn't do. I have a patchset pending that makes major changes to all this and totally conflicts with your code.

You can carry this for a while but I might revert your core code in my new patchset.

Sigh, more work for me.

Boaz

#19 lore raw
Quoted 8 lines · Boaz Harrosh
Quoted 2 lines · Benny Halevy

Yup, see patchset in reply.

That I wish you wouldn't do. I have a patchset pending that makes major changes to all this and totally conflicts with your code.

Sorry, I didn't know that.

Quoted 3 lines

You can carry this for a while but I might revert your core code in my new patchset.

I hope that at least it will all be for the better.

Benny

Quoted 8 lines

Sigh, more work for me.

Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#20 lore raw
Quoted 13 lines · Benny Halevy
Quoted 8 lines · Boaz Harrosh
Quoted 2 lines · Benny Halevy

Yup, see patchset in reply.

That I wish you wouldn't do. I have a patchset pending that makes major changes to all this and totally conflicts with your code.

Sorry, I didn't know that.

Quoted 1 line
Quoted 7 lines
Quoted 2 lines

You can carry this for a while but I might revert your core code in my new patchset.

I hope that at least it will all be for the better.

Benny

NP. Yes it is all for the better. I think you'll like what I did.

BTW after I finish implementing my stuff. I think it would be easy to implement that open_state_id thing. All we need is to save the open_state_id somewhere per-file or somehow identify it's receive, And then just call nfs4_roc() which will do the proper work.

But please wait for me to finish.

(It is all done and debugged, but will be divided and patched
on Wednesday)

Thanks
Boaz

#21 lore raw
Quoted 24 lines · Boaz Harrosh
Quoted 13 lines · Benny Halevy
Quoted 8 lines · Boaz Harrosh
Quoted 2 lines · Benny Halevy

Yup, see patchset in reply.

That I wish you wouldn't do. I have a patchset pending that makes major changes to all this and totally conflicts with your code.

Sorry, I didn't know that.

Quoted 1 line
Quoted 7 lines
Quoted 2 lines

You can carry this for a while but I might revert your core code in my new patchset.

I hope that at least it will all be for the better.

Benny

Quoted 6 lines

NP. Yes it is all for the better. I think you'll like what I did.

BTW after I finish implementing my stuff. I think it would be easy to implement that open_state_id thing. All we need is to save the open_state_id somewhere per-file or somehow identify it's receive, And then just call nfs4_roc() which will do the proper work.

It's any non layout stateid so it should be pretty easy (in theory :). See nfs4_process_layout_stateid()
if (stid->sc_type != NFS4_LAYOUT_STID) {}

We should be able to locate the layout stateid, if any, on the fp->fi_layout_states list by matching stid->sc_client.

Benny

Quoted 5 lines

But please wait for me to finish.

(It is all done and debugged, but will be divided and patched
on Wednesday)

Sure. Thanks!

Benny

Quoted 7 lines

Thanks
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#22 lore raw
Quoted 8 lines · Benny Halevy
Quoted 4 lines · Boaz Harrosh

BTW after I finish implementing my stuff. I think it would be easy to implement that open_state_id thing. All we need is to save the open_state_id somewhere per-file or somehow identify it's receive, And then just call nfs4_roc() which will do the proper work.

Quoted 7 lines

It's any non layout stateid so it should be pretty easy (in theory :). See nfs4_process_layout_stateid()
if (stid->sc_type != NFS4_LAYOUT_STID) {}

We should be able to locate the layout stateid, if any, on the fp->fi_layout_states list by matching stid->sc_client.

Does look easy, then. I thought it should be. I will have a shot at it and add it to my patchset.

Do you know if we have a test case for this in pyNFS?

I need to think if we can cause this with the Linux client?
Perhaps: not set ROC, access a file and close, clear caches at both ends, re access, something like that, I'll try.

Thanks
Boaz

#23 lore raw
Quoted 14 lines · Boaz Harrosh
Quoted 12 lines · Benny Halevy
Quoted 4 lines · Boaz Harrosh

BTW after I finish implementing my stuff. I think it would be easy to implement that open_state_id thing. All we need is to save the open_state_id somewhere per-file or somehow identify it's receive, And then just call nfs4_roc() which will do the proper work.

It's any non layout stateid so it should be pretty easy (in theory :). See nfs4_process_layout_stateid()
if (stid->sc_type != NFS4_LAYOUT_STID) {}

Quoted 7 lines
Quoted 3 lines

We should be able to locate the layout stateid, if any, on the fp->fi_layout_states list by matching stid->sc_client.

Does look easy, then. I thought it should be. I will have a shot at it and add it to my patchset.

Thanks!

Quoted 2 lines

Do you know if we have a test case for this in pyNFS?

No, not that I know of.

Quoted 4 lines

I need to think if we can cause this with the Linux client?
Perhaps: not set ROC, access a file and close, clear caches at both ends, re access, something like that, I'll try.

Cool. Thanks a bunch for picking this up!

Benny

Quoted 5 lines

Thanks
Boaz

--