Skip to contentSkip to conversation

Some more bugs in latest code + please help with last problem

PATCH New Boaz Harrosh 1 participant 4 messages May 24, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover Some more bugs in latest code + please help with last problem

I'm sending some more fixes.

After launch I will re-send all the fixes needed ontop of Benny's last tree as SQUASHMEs. One SQUASHME for each patch in the set that needs one.

Last sent fixes will have conflicts with newest code. So these re-sent will resolve that.

With this code, I'm able to use the global-device-cache the way It was with the old one. Also including all the fixes I'm able to do IO, and checkout a git tree!

*BUT*: Currently IO is very very slow, because wsize/rsize is set
to default 64k which is just one strip_unit for default objects. I used to set server->w/rsize on .set_layoutdriver but Benny removed that. What do you guys want to do with this? I can see to options:
1. Return the .set_layoutdriver where ld can set defaults the way it likes it.
2. If pnfs is used set w/rsize to MAX_UINT and let pg_test cap the sizes as see fit.

Please advise?

For now these:
SQUSHME: pnfs: BUG in _deviceid_purge_client
pnfs: layout_driver MUST set free_deviceid_node if using dev-cache
SQUASHME: pnfs-obj: objlayout wants to cache devices until unmount

Thanks
Boaz

#1 lore raw

PATCH 1/3 SQUSHME: pnfs: BUG in _deviceid_purge_client

The atomic_dec_and_test() returns *true* when zero. The ! belongs to atomic_dec(). Fixit

---
 fs/nfs/pnfs_dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/nfs/pnfs_dev.c +1 -1
index 7e5542c..37ca215 100644
@@ -258,7 +258,7 @@ _deviceid_purge_client(const struct nfs_client *clp, long hash)
 
 	synchronize_rcu();
 	hlist_for_each_entry_safe(d, n, next, &tmp, node)
-		if (!atomic_dec_and_test(&d->ref)) {
+		if (atomic_dec_and_test(&d->ref)) {
 			if (d->ld->free_deviceid_node)
 				d->ld->free_deviceid_node(d);
 			else
Signature · 1 line
1.7.2.3
#2 lore raw

PATCH 1/1 pnfs: layout_driver MUST set .free_deviceid_node if using dev-cache

In reply to Boaz Harrosh

A device cache is not a matter of memory store. It is a matter of mounting/login and unmounting/logout. So it is not logical to not set free_deviceid_node. Who will do the unmount?

It is better to crash the developer then let him leak mounts.

---
 fs/nfs/pnfs_dev.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)
fs/nfs/pnfs_dev.c +4 -12
index 37ca215..1b592d9 100644
@@ -163,10 +163,7 @@ nfs4_delete_deviceid(const struct pnfs_layoutdriver_type *ld,
 	d = nfs4_unhash_put_deviceid(ld, clp, id);
 	if (!d)
 		return;
-	if (d->ld->free_deviceid_node)
-		d->ld->free_deviceid_node(d);
-	else
-		kfree(d);
+	d->ld->free_deviceid_node(d);
 }
 EXPORT_SYMBOL_GPL(nfs4_delete_deviceid);
 
@@ -232,8 +229,7 @@ nfs4_put_deviceid_node(struct nfs4_deviceid_node *d)
 	hlist_del_init_rcu(&d->node);
 	spin_unlock(&nfs4_deviceid_lock);
 	synchronize_rcu();
-	if (d->ld->free_deviceid_node)
-		d->ld->free_deviceid_node(d);
+	d->ld->free_deviceid_node(d);
 	return true;
 }
 EXPORT_SYMBOL_GPL(nfs4_put_deviceid_node);
@@ -258,12 +254,8 @@ _deviceid_purge_client(const struct nfs_client *clp, long hash)
 
 	synchronize_rcu();
 	hlist_for_each_entry_safe(d, n, next, &tmp, node)
-		if (atomic_dec_and_test(&d->ref)) {
-			if (d->ld->free_deviceid_node)
-				d->ld->free_deviceid_node(d);
-			else
-				kfree(d);
-		}
+		if (atomic_dec_and_test(&d->ref))
+			d->ld->free_deviceid_node(d);
 }
 
 void
Signature · 1 line
1.7.2.3
#3 lore raw

PATCH 3/3 SQUASHME: pnfs-obj: objlayout wants to cache devices until unmount

In reply to Boaz Harrosh

Take an extra reference on a device insert. So devices keep around in the cache until nfs_client release. (This was the behaviour of the old cache)

The extra reference will be removed in nfs4_deviceid_purge_client(). I tested this and it works perfectly.

TODO: Define an nfs4_get_deviceid()
Currently accessing did->ref directly

TODO: nfs4_insert_deviceid_node should check if there are too many
devices and start purging them. Say by longest time from last use.

---
 fs/nfs/objlayout/objio_osd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
fs/nfs/objlayout/objio_osd.c +1 -0
index c09066f..47e8cb5 100644
@@ -110,6 +110,7 @@ _dev_list_add(const struct nfs_server *nfss,
 		de = n;
 	}
 
+	atomic_inc(&de->id_node.ref);
 	return de;
 }
 
Signature · 1 line
1.7.2.3