[PATCH] Silence compiler warning

[PATCH] Silence compiler warning

[Jim Rees]

fs/nfs/callback_proc.c: In function ‘do_callback_layoutrecall’:
fs/nfs/callback_proc.c:115:26: warning: ‘lo’ may be used uninitialized in this function

No functional change. If no layout is found, we'll return before  using
"lo".

Signed-off-by: Jim Rees <rees@umich.edu>
---
 fs/nfs/callback_proc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index 43926ad..93633f1 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -112,7 +112,7 @@ static u32 initiate_file_draining(struct nfs_client *clp,
 				  struct cb_layoutrecallargs *args)
 {
 	struct nfs_server *server;
-	struct pnfs_layout_hdr *lo;
+	struct pnfs_layout_hdr *lo = NULL;
 	struct inode *ino;
 	bool found = false;
 	u32 rv = NFS4ERR_NOMATCHING_LAYOUT;
-- 
1.7.4.1

Re: [PATCH] Silence compiler warning

[Benny Halevy]

On 2011-10-16 17:06, Jim Rees wrote:
> fs/nfs/callback_proc.c: In function ‘do_callback_layoutrecall’:
> fs/nfs/callback_proc.c:115:26: warning: ‘lo’ may be used uninitialized in this function
> 
> No functional change. If no layout is found, we'll return before  using
> "lo".
> 
> Signed-off-by: Jim Rees <rees@umich.edu>
> ---
>  fs/nfs/callback_proc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> index 43926ad..93633f1 100644
> --- a/fs/nfs/callback_proc.c
> +++ b/fs/nfs/callback_proc.c
> @@ -112,7 +112,7 @@ static u32 initiate_file_draining(struct nfs_client *clp,
>  				  struct cb_layoutrecallargs *args)
>  {
>  	struct nfs_server *server;
> -	struct pnfs_layout_hdr *lo;
> +	struct pnfs_layout_hdr *lo = NULL;
>  	struct inode *ino;
>  	bool found = false;
>  	u32 rv = NFS4ERR_NOMATCHING_LAYOUT;

Hmm, the warning seems bogus since we use lo only iff found==true
and it is set iff found==true
I wonder why I don't see that warning.
What compiler/version are you using?
At any rate, this made me think of a cleaner way to implement this:

git diff --stat -p -M
 fs/nfs/callback_proc.c |   56 ++++++++++++++++++++++++++++++++---------------
 1 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index 43926ad..e8d83a7 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -108,42 +108,62 @@ int nfs4_validate_delegation_stateid(struct nfs_delegation *delegation, const nf

 #if defined(CONFIG_NFS_V4_1)

-static u32 initiate_file_draining(struct nfs_client *clp,
-				  struct cb_layoutrecallargs *args)
+/*
+ * Lookup a layout by filehandle.
+ *
+ * Note: gets a refcount on the layout hdr and on its respective inode.
+ * Caller must put the layout hdr and the inode.
+ *
+ * TODO: keep track of all layouts (and delegations) in a hash table
+ * hashed by filehandle.
+ */
+static struct pnfs_layout_hdr * get_layout_by_fh_locked(struct nfs_client *clp, struct nfs_fh *fh)
 {
 	struct nfs_server *server;
-	struct pnfs_layout_hdr *lo;
 	struct inode *ino;
-	bool found = false;
-	u32 rv = NFS4ERR_NOMATCHING_LAYOUT;
-	LIST_HEAD(free_me_list);
+	struct pnfs_layout_hdr *lo;

-	spin_lock(&clp->cl_lock);
-	rcu_read_lock();
 	list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link) {
 		list_for_each_entry(lo, &server->layouts, plh_layouts) {
-			if (nfs_compare_fh(&args->cbl_fh,
-					   &NFS_I(lo->plh_inode)->fh))
+			if (nfs_compare_fh(fh, &NFS_I(lo->plh_inode)->fh))
 				continue;
 			ino = igrab(lo->plh_inode);
 			if (!ino)
 				continue;
-			found = true;
-			/* Without this, layout can be freed as soon
-			 * as we release cl_lock.
-			 */
 			get_layout_hdr(lo);
-			break;
+			return lo;
 		}
-		if (found)
-			break;
 	}
+
+	return NULL;
+}
+
+static struct pnfs_layout_hdr * get_layout_by_fh(struct nfs_client *clp, struct nfs_fh *fh)
+{
+	struct pnfs_layout_hdr *lo;
+
+	spin_lock(&clp->cl_lock);
+	rcu_read_lock();
+	lo = get_layout_by_fh_locked(clp, fh);
 	rcu_read_unlock();
 	spin_unlock(&clp->cl_lock);

-	if (!found)
+	return lo;
+}
+
+static u32 initiate_file_draining(struct nfs_client *clp,
+				  struct cb_layoutrecallargs *args)
+{
+	struct inode *ino;
+	struct pnfs_layout_hdr *lo;
+	u32 rv = NFS4ERR_NOMATCHING_LAYOUT;
+	LIST_HEAD(free_me_list);
+
+	lo = get_layout_by_fh(clp, &args->cbl_fh);
+	if (!lo)
 		return NFS4ERR_NOMATCHING_LAYOUT;

+	ino = lo->plh_inode;
 	spin_lock(&ino->i_lock);
 	if (test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags) ||
 	    mark_matching_lsegs_invalid(lo, &free_me_list,

Re: [PATCH] Silence compiler warning

[Jim Rees]

Benny Halevy wrote:

  On 2011-10-16 17:06, Jim Rees wrote:
  > fs/nfs/callback_proc.c: In function ‘do_callback_layoutrecall’:
  > fs/nfs/callback_proc.c:115:26: warning: ‘lo’ may be used uninitialized in this function
  > 
  > No functional change. If no layout is found, we'll return before  using
  > "lo".
  > 
  > Signed-off-by: Jim Rees <rees@umich.edu>
  > ---
  >  fs/nfs/callback_proc.c |    2 +-
  >  1 files changed, 1 insertions(+), 1 deletions(-)
  > 
  > diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
  > index 43926ad..93633f1 100644
  > --- a/fs/nfs/callback_proc.c
  > +++ b/fs/nfs/callback_proc.c
  > @@ -112,7 +112,7 @@ static u32 initiate_file_draining(struct nfs_client *clp,
  >  				  struct cb_layoutrecallargs *args)
  >  {
  >  	struct nfs_server *server;
  > -	struct pnfs_layout_hdr *lo;
  > +	struct pnfs_layout_hdr *lo = NULL;
  >  	struct inode *ino;
  >  	bool found = false;
  >  	u32 rv = NFS4ERR_NOMATCHING_LAYOUT;
  
  Hmm, the warning seems bogus since we use lo only iff found==true
  and it is set iff found==true
  I wonder why I don't see that warning.
  What compiler/version are you using?

gcc (Ubuntu/Linaro 4.5.2-8ubuntu4) 4.5.2

I don't remember seeing this warning before either, but I can't think what I
might have changed that would make a difference.  I did turn on SMP, which I
didn't have before (non-SMP kernels don't seem to work on the latest
Virtualbox).  And yes, the warning is bogus, but should be fixed anyway.

Re: [PATCH] Silence compiler warning

[Stanislav Kinsbursky]

I have this warning also:
fs/nfs/callback_proc.c:116: warning: ‘ino’ may be used uninitialized in this 
function

17.10.2011 04:06, Jim Rees пишет:
> fs/nfs/callback_proc.c: In function ‘do_callback_layoutrecall’:
> fs/nfs/callback_proc.c:115:26: warning: ‘lo’ may be used uninitialized in this function
>
> No functional change. If no layout is found, we'll return before  using
> "lo".
>
> Signed-off-by: Jim Rees<rees@umich.edu>
> ---
>   fs/nfs/callback_proc.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> index 43926ad..93633f1 100644
> --- a/fs/nfs/callback_proc.c
> +++ b/fs/nfs/callback_proc.c
> @@ -112,7 +112,7 @@ static u32 initiate_file_draining(struct nfs_client *clp,
>   				  struct cb_layoutrecallargs *args)
>   {
>   	struct nfs_server *server;
> -	struct pnfs_layout_hdr *lo;
> +	struct pnfs_layout_hdr *lo = NULL;
>   	struct inode *ino;
>   	bool found = false;
>   	u32 rv = NFS4ERR_NOMATCHING_LAYOUT;


-- 
Best regards,
Stanislav Kinsbursky

Re: [PATCH] Silence compiler warning

[Benny Halevy]

T2suIEkgY29tbWl0dGVkIHRoZSBjbGVhbnVwIHBhdGNoIEkgc2VudCBvbiB0aGlzIHRocmVhZCBh
bmQgcHVzaGVkIGl0IG91dCB5ZXN0ZXJkYXkuDQpUaGlzIHdhcm5pbmcgc2hvdWxkIG5vdyBiZSBn
b25lLg0KQmVubnkNCi0tLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLS0NCkZyb206IFN0YW5pc2xh
diBLaW5zYnVyc2t5DQpUbzogSmltIFJlZXMNCkNjOiBCZW5ueSBIYWxldnkNCkNjOiBsaW51eC1u
ZnNAdmdlci5rZXJuZWwub3JnDQpTdWJqZWN0OiBSZTogW1BBVENIXSBTaWxlbmNlIGNvbXBpbGVy
IHdhcm5pbmcNClNlbnQ6IE9jdCAxOCwgMjAxMSAwMjo1MA0KDQpJIGhhdmUgdGhpcyB3YXJuaW5n
IGFsc286DQpmcy9uZnMvY2FsbGJhY2tfcHJvYy5jOjExNjogd2FybmluZzog4oCYaW5v4oCZIG1h
eSBiZSB1c2VkIHVuaW5pdGlhbGl6ZWQgaW4gdGhpcyANCmZ1bmN0aW9uDQoNCjE3LjEwLjIwMTEg
MDQ6MDYsIEppbSBSZWVzINC/0LjRiNC10YI6DQo+IGZzL25mcy9jYWxsYmFja19wcm9jLmM6IElu
IGZ1bmN0aW9uIOKAmGRvX2NhbGxiYWNrX2xheW91dHJlY2FsbOKAmToNCj4gZnMvbmZzL2NhbGxi
YWNrX3Byb2MuYzoxMTU6MjY6IHdhcm5pbmc6IOKAmGxv4oCZIG1heSBiZSB1c2VkIHVuaW5pdGlh
bGl6ZWQgaW4gdGhpcyBmdW5jdGlvbg0KPg0KPiBObyBmdW5jdGlvbmFsIGNoYW5nZS4gSWYgbm8g
bGF5b3V0IGlzIGZvdW5kLCB3ZSdsbCByZXR1cm4gYmVmb3JlICB1c2luZw0KPiAibG8iLg0KPg0K
PiBTaWduZWQtb2ZmLWJ5OiBKaW0gUmVlczxyZWVzQHVtaWNoLmVkdT4NCj4gLS0tDQo+ICAgZnMv
bmZzL2NhbGxiYWNrX3Byb2MuYyB8ICAgIDIgKy0NCj4gICAxIGZpbGVzIGNoYW5nZWQsIDEgaW5z
ZXJ0aW9ucygrKSwgMSBkZWxldGlvbnMoLSkNCj4NCj4gZGlmZiAtLWdpdCBhL2ZzL25mcy9jYWxs
YmFja19wcm9jLmMgYi9mcy9uZnMvY2FsbGJhY2tfcHJvYy5jDQo+IGluZGV4IDQzOTI2YWQuLjkz
NjMzZjEgMTAwNjQ0DQo+IC0tLSBhL2ZzL25mcy9jYWxsYmFja19wcm9jLmMNCj4gKysrIGIvZnMv
bmZzL2NhbGxiYWNrX3Byb2MuYw0KPiBAQCAtMTEyLDcgKzExMiw3IEBAIHN0YXRpYyB1MzIgaW5p
dGlhdGVfZmlsZV9kcmFpbmluZyhzdHJ1Y3QgbmZzX2NsaWVudCAqY2xwLA0KPiAgIAkJCQkgIHN0
cnVjdCBjYl9sYXlvdXRyZWNhbGxhcmdzICphcmdzKQ0KPiAgIHsNCj4gICAJc3RydWN0IG5mc19z
ZXJ2ZXIgKnNlcnZlcjsNCj4gLQlzdHJ1Y3QgcG5mc19sYXlvdXRfaGRyICpsbzsNCj4gKwlzdHJ1
Y3QgcG5mc19sYXlvdXRfaGRyICpsbyA9IE5VTEw7DQo+ICAgCXN0cnVjdCBpbm9kZSAqaW5vOw0K
PiAgIAlib29sIGZvdW5kID0gZmFsc2U7DQo+ICAgCXUzMiBydiA9IE5GUzRFUlJfTk9NQVRDSElO
R19MQVlPVVQ7DQoNCg0KLS0gDQpCZXN0IHJlZ2FyZHMsDQpTdGFuaXNsYXYgS2luc2J1cnNreQ0K
DQo=

[PATCH] Silence compiler warning

[Jim Rees]

I think this is still needed, isn't it?  I haven't tried compiling
nfs-for-next but I don't see a fix for it in there.

From: Jim Rees <rees@umich.edu>
Date: Sun, 16 Oct 2011 20:06:52 -0400
Subject: [PATCH] Silence compiler warning

fs/nfs/callback_proc.c: In function ‘do_callback_layoutrecall’:
fs/nfs/callback_proc.c:115:26: warning: ‘lo’ may be used uninitialized in this function

No functional change. If no layout is found, we'll return before  using
"lo".

Signed-off-by: Jim Rees <rees@umich.edu>
---
 fs/nfs/callback_proc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index 43926ad..f384eb1 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -112,8 +112,8 @@ static u32 initiate_file_draining(struct nfs_client *clp,
 				  struct cb_layoutrecallargs *args)
 {
 	struct nfs_server *server;
-	struct pnfs_layout_hdr *lo;
-	struct inode *ino;
+	struct pnfs_layout_hdr *lo = NULL;
+	struct inode *ino = NULL;
 	bool found = false;
 	u32 rv = NFS4ERR_NOMATCHING_LAYOUT;
 	LIST_HEAD(free_me_list);
-- 
1.7.4.1

Re: [PATCH] Silence compiler warning

[Trond Myklebust]

On Mon, 2011-10-31 at 21:37 -0400, Jim Rees wrote: 
> I think this is still needed, isn't it?  I haven't tried compiling
> nfs-for-next but I don't see a fix for it in there.

AFAICS, this is a bogus warning: if lo is uninitialised, we will
automatically exit with a return value of NFS4ERR_NOMATCHING_LAYOUT.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] Silence compiler warning

[Jim Rees]

Trond Myklebust wrote:

  On Mon, 2011-10-31 at 21:37 -0400, Jim Rees wrote: 
  > I think this is still needed, isn't it?  I haven't tried compiling
  > nfs-for-next but I don't see a fix for it in there.
  
  AFAICS, this is a bogus warning: if lo is uninitialised, we will
  automatically exit with a return value of NFS4ERR_NOMATCHING_LAYOUT.

Yes, the warning is bogus.  I just want the damn compiler to shut up so I
can see if there are any real warnings.

Re: [PATCH] Silence compiler warning

[Trond Myklebust]

On Mon, 2011-10-31 at 22:42 -0400, Jim Rees wrote: 
> Trond Myklebust wrote:
> 
>   On Mon, 2011-10-31 at 21:37 -0400, Jim Rees wrote: 
>   > I think this is still needed, isn't it?  I haven't tried compiling
>   > nfs-for-next but I don't see a fix for it in there.
>   
>   AFAICS, this is a bogus warning: if lo is uninitialised, we will
>   automatically exit with a return value of NFS4ERR_NOMATCHING_LAYOUT.
> 
> Yes, the warning is bogus.  I just want the damn compiler to shut up so I
> can see if there are any real warnings.

How about cleaning up the whole unnecessary "bool found" crap that has
it confused? You can easily replace that with a test for 'ino != NULL'.
We don't usually "fix" compiler bugs by changing valid kernel code, but
cleanups are acceptable if they help to clarify what is going on.

Oh, another thing: that code will currently race very nicely against
'umount', and looks capable of triggering the 'VFS: Busy inodes after
unmount of foo. Self-destruct in 5 seconds.  Have a nice day..." since
it doesn't do anything to pin the super block while holding a reference
to the inode.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] Silence compiler warning

[Jim Rees]

Trond Myklebust wrote:

  On Mon, 2011-10-31 at 22:42 -0400, Jim Rees wrote: 
  > Trond Myklebust wrote:
  > 
  >   On Mon, 2011-10-31 at 21:37 -0400, Jim Rees wrote: 
  >   > I think this is still needed, isn't it?  I haven't tried compiling
  >   > nfs-for-next but I don't see a fix for it in there.
  >   
  >   AFAICS, this is a bogus warning: if lo is uninitialised, we will
  >   automatically exit with a return value of NFS4ERR_NOMATCHING_LAYOUT.
  > 
  > Yes, the warning is bogus.  I just want the damn compiler to shut up so I
  > can see if there are any real warnings.
  
  How about cleaning up the whole unnecessary "bool found" crap that has
  it confused? You can easily replace that with a test for 'ino != NULL'.
  We don't usually "fix" compiler bugs by changing valid kernel code, but
  cleanups are acceptable if they help to clarify what is going on.
  
  Oh, another thing: that code will currently race very nicely against
  'umount', and looks capable of triggering the 'VFS: Busy inodes after
  unmount of foo. Self-destruct in 5 seconds.  Have a nice day..." since
  it doesn't do anything to pin the super block while holding a reference
  to the inode.

There's a patch in Benny's tree that I think addresses these issues in
addition to the compiler warnings.  Maybe Benny would like to move it
forward?