As promised I'm resending all accumulated fixes from Benny's last release.
They have been produced in the below order, each fix right after the patch
that it needs to squash into.
* I have split those SQUASHMEs that belong to different patches.
* I have reordered some of benny's patchset and also split one
patch into two. A patch that had both generic and objects code
in it. I will resend also these patches the one that got split
and a one that was reordered. Please see below.
Benny, Trond If you need to I can send the all ready-made patchset
on call. It is right here waiting. If any body needs it. (Just didn't
want to spam the list.
Please don't forget to throw away all the previous, fixes I sent this
includes everything cleaned up.
It is all based on Benny's: [PATCHSET v6 0/26] pnfs for 2.6.40
Here below is the new patchset order interleaved with the patches
I'm resending. Please apply them in that order and SQUASH
These patches that are indented in are left the same as before:
#pick 87482a8 NFSv4.1: use struct nfs_client to qualify deviceid
#pick 21a93b1 pnfs: resolve header dependency in pnfs.h
#pick d322d23 NFSv4.1: make deviceid cache global
#pick e96d556 NFSv4.1: purge deviceid cache on nfs_free_client
#pick 839fb76 pnfs: CB_NOTIFY_DEVICEID
pick b97ee18 NFSv4.1: use layout driver in global device cache - Resend
I'm resending this because I split out the objects code
to the relevant patch below (I've put it here so to apply the
fixes)
pick 898c6d6 SQUASHME: Bug in new global-device-cache code
pick c04e8c7 SQUSHME: pnfs: BUG in _deviceid_purge_client
These two are a resend and a cleanup of the bug fixes I sent
pick c92b213 pnfs: layout_driver MUST set free_deviceid_node if using dev-cache
This one I also sent. It is not a SQUASHME it needs to be added
here
#pick d1dc744 SUNRPC: introduce xdr_init_decode_pages
#pick 020333f pnfs: Use byte-range for layoutget
#pick 50a623c pnfs: align layoutget requests on page boundaries
#pick 899e1e1 pnfs: Use byte-range for cb_layoutrecall
#pick 24430f8 pnfs: client stats
#pick e005007 pnfs-obj: objlayoutdriver module skeleton
#pick e679f1b pnfs-obj: pnfs_osd XDR definitions
pick d1fb331 SQUASHME: pnfs-obj: pnfs_osd_xdr.h Remove server definitions
Resend + more removed
#pick 98f3079 pnfs-obj: pnfs_osd XDR client implementation
pick a6d6418 SQUASHME: pnf-obj xdr_cli: Wrong type in comments
Resend
#pick c7b5197 pnfs-obj: decode layout, alloc/free lseg
pick 619d431 SQUASHME: objio alloc/free lseg Bugs fixes
Resend
#pick 756866c pnfs-obj: objio_osd device information retrieval and caching
pick 45f1e63 SQUASHME: pnfs-obj: use layout driver in global device cache
pick b8b9d0e SQUASHME: pnfs-obj: Bugs in new global-device-cache code
pick d2bff4d SQUASHME: pnfs-obj: objlayout wants to cache devices until unmount
All these where sent in different patches, here divided properly to
apply cleanly to the original patch.
#pick 2f7beef pnfs: alloc and free layout_hdr layoutdriver methods
#pick 603ceb3 pnfs-obj: define per-inode private structure
#pick 608f381 pnfs: support for non-rpc layout drivers
pick 8be3eb4 SQUASHME: pnfs: Fall out from: non-rpc layout drivers
Resend
#pick 558f335 pnfs-obj: osd raid engine read/write implementation
pick c437fbb SQUASHME: objio read/write patch: Bugs fixes
Resend properly rebased this time
#pick db53862 pnfs: layoutreturn
#pick 6e323de pnfs: layoutret_on_setattr
#pick 385cc07 pnfs: encode_layoutreturn
#pick 8eb75c9 pnfs-obj: report errors and .encode_layoutreturn Implementation.
#pick e5e510b pnfs: encode_layoutcommit
#pick bf2158f pnfs-obj: objlayout_encode_layoutcommit implementation
NULL deref on first ever call. (When device is not found)
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/nfs/pnfs_dev.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
index 7997899..7e5542c 100644
--- a/fs/nfs/pnfs_dev.c
+++ b/fs/nfs/pnfs_dev.c
@@ -100,7 +100,7 @@ _find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
rcu_read_lock();
d = _lookup_deviceid(ld, clp, id, hash);
- if (!atomic_inc_not_zero(&d->ref))
+ if (!d || !atomic_inc_not_zero(&d->ref))
d = NULL;
rcu_read_unlock();
return d;
--
1.7.2.3
On 2011-05-24 18:07, Boaz Harrosh wrote:
> Fix BUGs in the new "Use global-device-cache".
>
> One thing I don't understand is why the compiler did
> not complain when the code was returning the wrong
> type of structure
>
> Signed-off-by: Boaz Harrosh <[email protected]>
> ---
> fs/nfs/objlayout/objio_osd.c | 28 +++++++++++++++++++---------
> 1 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
> index 167cd1e..faacde2 100644
> --- a/fs/nfs/objlayout/objio_osd.c
> +++ b/fs/nfs/objlayout/objio_osd.c
> @@ -56,6 +56,7 @@ objio_free_deviceid_node(struct nfs4_deviceid_node *d)
> {
> struct objio_dev_ent *de = container_of(d, struct objio_dev_ent, id_node);
>
> + dprintk("%s: free od=%p\n", __func__, de->od);
> osduld_put_device(de->od);
> kfree(de);
> }
> @@ -64,14 +65,18 @@ static struct objio_dev_ent *_dev_list_find(const struct nfs_server *nfss,
> const struct nfs4_deviceid *d_id)
> {
> struct nfs4_deviceid_node *d;
> + struct objio_dev_ent *de;
>
> d = nfs4_find_get_deviceid(nfss->pnfs_curr_ld, nfss->nfs_client, d_id);
> if (!d)
> return NULL;
> - return container_of(d, struct objio_dev_ent, id_node);
> +
> + de = container_of(d, struct objio_dev_ent, id_node);
> + return de;
That's not really required as container_of() does the type casting
for you.
> }
>
> -static int _dev_list_add(const struct nfs_server *nfss,
> +static struct objio_dev_ent *
> +_dev_list_add(const struct nfs_server *nfss,
> const struct nfs4_deviceid *d_id, struct osd_dev *od,
> gfp_t gfp_flags)
> {
> @@ -79,9 +84,12 @@ static int _dev_list_add(const struct nfs_server *nfss,
> struct objio_dev_ent *de = kzalloc(sizeof(*de), gfp_flags);
> struct objio_dev_ent *n;
>
> - if (!de)
> - return -ENOMEM;
> + if (!de) {
> + dprintk("%s: -ENOMEM od=%p\n", __func__, od);
> + return NULL;
better return ERR_PTR(-ENOMEM)
that will percolate up the stack via _device_lookup.
Thanks!
Benny
> + }
>
> + dprintk("%s: Adding od=%p\n", __func__, od);
> nfs4_init_deviceid_node(&de->id_node,
> nfss->pnfs_curr_ld,
> nfss->nfs_client,
> @@ -91,11 +99,12 @@ static int _dev_list_add(const struct nfs_server *nfss,
> d = nfs4_insert_deviceid_node(&de->id_node);
> n = container_of(d, struct objio_dev_ent, id_node);
> if (n != de) {
> - BUG_ON(n->od != od);
> + dprintk("%s: Race with other n->od=%p\n", __func__, n->od);
> objio_free_deviceid_node(&de->id_node);
> + de = n;
> }
>
> - return 0;
> + return de;
> }
>
> struct caps_buffers {
> @@ -117,7 +126,7 @@ struct objio_segment {
> unsigned comps_index;
> unsigned num_comps;
> /* variable length */
> - struct objio_dev_ent *ods[0];
> + struct objio_dev_ent *ods[];
> };
>
> static inline struct objio_segment *
> @@ -176,12 +185,13 @@ static struct objio_dev_ent *_device_lookup(struct pnfs_layout_hdr *pnfslay,
> goto out;
> }
>
> - _dev_list_add(NFS_SERVER(pnfslay->plh_inode), d_id, od, gfp_flags);
> + ode = _dev_list_add(NFS_SERVER(pnfslay->plh_inode), d_id, od,
> + gfp_flags);
>
> out:
> dprintk("%s: return=%d\n", __func__, err);
> objlayout_put_deviceinfo(deviceaddr);
> - return err ? ERR_PTR(err) : od;
> + return err ? ERR_PTR(err) : ode;
> }
>
> static int objio_devices_lookup(struct pnfs_layout_hdr *pnfslay,
Take an extra reference on a device insert. So devices keep
around in the cache until nfs_client release.
(This was the behavior 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 accesing did->ref directly
TODO: nfs4_insert_deviceid_node should check if there are too many
devices and start purging them. Say by time from last use.
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/nfs/objlayout/objio_osd.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
index faacde2..8b05b16 100644
--- a/fs/nfs/objlayout/objio_osd.c
+++ b/fs/nfs/objlayout/objio_osd.c
@@ -104,6 +104,7 @@ _dev_list_add(const struct nfs_server *nfss,
de = n;
}
+ atomic_inc(&de->id_node.ref);
return de;
}
--
1.7.2.3
The atomic_dec_and_test() returns *true* when zero. The !
belongs to atomic_dec(). Fixit
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/nfs/pnfs_dev.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
index 7e5542c..37ca215 100644
--- a/fs/nfs/pnfs_dev.c
+++ b/fs/nfs/pnfs_dev.c
@@ -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
--
1.7.2.3
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.
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/nfs/pnfs_dev.c | 16 ++++------------
1 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
index 37ca215..1b592d9 100644
--- a/fs/nfs/pnfs_dev.c
+++ b/fs/nfs/pnfs_dev.c
@@ -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
--
1.7.2.3
From: Benny Halevy <[email protected]>
This is the objio_osd part of the patch:
NFSv4.1: use layout driver in global device cache
I have move that patch to the begining of the submit
toghther with the bug fixing of the new code
This here should just be squashed into the single objlayout
part of the get_devic_info handling
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfs/objlayout/objio_osd.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
index bcc8468..a4201d8 100644
--- a/fs/nfs/objlayout/objio_osd.c
+++ b/fs/nfs/objlayout/objio_osd.c
@@ -60,12 +60,12 @@ objio_free_deviceid_node(struct nfs4_deviceid_node *d)
kfree(de);
}
-static struct objio_dev_ent *_dev_list_find(const struct nfs_client *clp,
+static struct objio_dev_ent *_dev_list_find(const struct nfs_server *nfss,
const struct nfs4_deviceid *d_id)
{
struct nfs4_deviceid_node *d;
- d = nfs4_find_get_deviceid(clp, d_id);
+ d = nfs4_find_get_deviceid(nfss->pnfs_curr_ld, nfss->nfs_client, d_id);
if (!d)
return NULL;
return container_of(d, struct objio_dev_ent, id_node);
@@ -141,7 +141,7 @@ static struct objio_dev_ent *_device_lookup(struct pnfs_layout_hdr *pnfslay,
d_id = &objio_seg->comps[comp].oc_object_id.oid_device_id;
- ode = _dev_list_find(NFS_SERVER(pnfslay->plh_inode)->nfs_client, d_id);
+ ode = _dev_list_find(NFS_SERVER(pnfslay->plh_inode), d_id);
if (ode)
return ode;
--
1.7.2.3
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/nfs/objlayout/pnfs_osd_xdr_cli.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/objlayout/pnfs_osd_xdr_cli.c b/fs/nfs/objlayout/pnfs_osd_xdr_cli.c
index dd3052c..16fc758 100644
--- a/fs/nfs/objlayout/pnfs_osd_xdr_cli.c
+++ b/fs/nfs/objlayout/pnfs_osd_xdr_cli.c
@@ -47,7 +47,7 @@
/*
* struct pnfs_osd_objid {
- * struct pnfs_deviceid oid_device_id;
+ * struct nfs4_deviceid oid_device_id;
* u64 oid_partition_id;
* u64 oid_object_id;
* }; // xdr size 32 bytes
@@ -366,7 +366,7 @@ pnfs_osd_xdr_encode_layoutupdate(struct xdr_stream *xdr,
/*
* struct pnfs_osd_objid {
- * struct pnfs_deviceid oid_device_id;
+ * struct nfs4_deviceid oid_device_id;
* u64 oid_partition_id;
* u64 oid_object_id;
* }; // xdr size 32 bytes
--
1.7.2.3
These are later added by the Server patchset
Signed-off-by: Boaz Harrosh <[email protected]>
---
include/linux/pnfs_osd_xdr.h | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/include/linux/pnfs_osd_xdr.h b/include/linux/pnfs_osd_xdr.h
index 747d06d..76efbdd 100644
--- a/include/linux/pnfs_osd_xdr.h
+++ b/include/linux/pnfs_osd_xdr.h
@@ -336,15 +336,10 @@ extern void pnfs_osd_xdr_decode_deviceaddr(
extern int
pnfs_osd_xdr_encode_layoutupdate(struct xdr_stream *xdr,
struct pnfs_osd_layoutupdate *lou);
-extern __be32 *
-pnfs_osd_xdr_decode_layoutupdate(struct pnfs_osd_layoutupdate *lou, __be32 *p);
/* osd_ioerror encoding/decoding (layout_return) */
/* Client */
extern __be32 *pnfs_osd_xdr_ioerr_reserve_space(struct xdr_stream *xdr);
extern void pnfs_osd_xdr_encode_ioerr(__be32 *p, struct pnfs_osd_ioerr *ioerr);
-/* Server*/
-extern __be32 *
-pnfs_osd_xdr_decode_ioerr(struct pnfs_osd_ioerr *ioerr, __be32 *p);
#endif /* __PNFS_OSD_XDR_H__ */
--
1.7.2.3
pnfs deviceids are unique per server, per layout type.
struct nfs_client is currently used to distinguish deviceids from
different nfs servers, yet these may clash between different layout
types on the same server. Therefore, use the layout driver associated
with each deviceid at insertion time to look it up, unhash, or
delete it.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfs/callback_proc.c | 2 +-
fs/nfs/nfs4filelayout.c | 3 ++-
fs/nfs/pnfs.h | 6 +++---
fs/nfs/pnfs_dev.c | 28 +++++++++++++++++-----------
4 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index fb5e5b9..c73e7b2 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -278,7 +278,7 @@ __be32 nfs4_callback_devicenotify(struct cb_devicenotifyargs *args,
if (dev->cbd_notify_type == NOTIFY_DEVICEID4_CHANGE)
dprintk("%s: NOTIFY_DEVICEID4_CHANGE not supported, "
"deleting instead\n", __func__);
- nfs4_delete_deviceid(clp, &dev->cbd_dev_id);
+ nfs4_delete_deviceid(server->pnfs_curr_ld, clp, &dev->cbd_dev_id);
}
out:
diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index c181a8b..45866d0 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -441,7 +441,8 @@ filelayout_check_layout(struct pnfs_layout_hdr *lo,
}
/* find and reference the deviceid */
- d = nfs4_find_get_deviceid(NFS_SERVER(lo->plh_inode)->nfs_client, id);
+ d = nfs4_find_get_deviceid(NFS_SERVER(lo->plh_inode)->pnfs_curr_ld,
+ NFS_SERVER(lo->plh_inode)->nfs_client, id);
if (d == NULL) {
dsaddr = get_device_info(lo->plh_inode, id, gfp_flags);
if (dsaddr == NULL)
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 7f29e3a..b7f88d4 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -171,9 +171,9 @@ struct nfs4_deviceid_node {
};
void nfs4_print_deviceid(const struct nfs4_deviceid *dev_id);
-struct nfs4_deviceid_node *nfs4_find_get_deviceid(const struct nfs_client *, const struct nfs4_deviceid *);
-struct nfs4_deviceid_node *nfs4_unhash_put_deviceid(const struct nfs_client *, const struct nfs4_deviceid *);
-void nfs4_delete_deviceid(const struct nfs_client *, const struct nfs4_deviceid *);
+struct nfs4_deviceid_node *nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *, const struct nfs_client *, const struct nfs4_deviceid *);
+struct nfs4_deviceid_node *nfs4_unhash_put_deviceid(const struct pnfs_layoutdriver_type *, const struct nfs_client *, const struct nfs4_deviceid *);
+void nfs4_delete_deviceid(const struct pnfs_layoutdriver_type *, const struct nfs_client *, const struct nfs4_deviceid *);
void nfs4_init_deviceid_node(struct nfs4_deviceid_node *,
const struct pnfs_layoutdriver_type *,
const struct nfs_client *,
diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
index f830616..7997899 100644
--- a/fs/nfs/pnfs_dev.c
+++ b/fs/nfs/pnfs_dev.c
@@ -67,14 +67,16 @@ nfs4_deviceid_hash(const struct nfs4_deviceid *id)
}
static struct nfs4_deviceid_node *
-_lookup_deviceid(const struct nfs_client *clp, const struct nfs4_deviceid *id,
+_lookup_deviceid(const struct pnfs_layoutdriver_type *ld,
+ const struct nfs_client *clp, const struct nfs4_deviceid *id,
long hash)
{
struct nfs4_deviceid_node *d;
struct hlist_node *n;
hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[hash], node)
- if (d->nfs_client == clp && !memcmp(&d->deviceid, id, sizeof(*id))) {
+ if (d->ld == ld && d->nfs_client == clp &&
+ !memcmp(&d->deviceid, id, sizeof(*id))) {
if (atomic_read(&d->ref))
return d;
else
@@ -90,13 +92,14 @@ _lookup_deviceid(const struct nfs_client *clp, const struct nfs4_deviceid *id,
* @id deviceid to look up
*/
struct nfs4_deviceid_node *
-_find_get_deviceid(const struct nfs_client *clp, const struct nfs4_deviceid *id,
+_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
+ const struct nfs_client *clp, const struct nfs4_deviceid *id,
long hash)
{
struct nfs4_deviceid_node *d;
rcu_read_lock();
- d = _lookup_deviceid(clp, id, hash);
+ d = _lookup_deviceid(ld, clp, id, hash);
if (!atomic_inc_not_zero(&d->ref))
d = NULL;
rcu_read_unlock();
@@ -104,9 +107,10 @@ _find_get_deviceid(const struct nfs_client *clp, const struct nfs4_deviceid *id,
}
struct nfs4_deviceid_node *
-nfs4_find_get_deviceid(const struct nfs_client *clp, const struct nfs4_deviceid *id)
+nfs4_find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
+ const struct nfs_client *clp, const struct nfs4_deviceid *id)
{
- return _find_get_deviceid(clp, id, nfs4_deviceid_hash(id));
+ return _find_get_deviceid(ld, clp, id, nfs4_deviceid_hash(id));
}
EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
@@ -119,13 +123,14 @@ EXPORT_SYMBOL_GPL(nfs4_find_get_deviceid);
* @ret the unhashed node, if found and dereferenced to zero, NULL otherwise.
*/
struct nfs4_deviceid_node *
-nfs4_unhash_put_deviceid(const struct nfs_client *clp, const struct nfs4_deviceid *id)
+nfs4_unhash_put_deviceid(const struct pnfs_layoutdriver_type *ld,
+ const struct nfs_client *clp, const struct nfs4_deviceid *id)
{
struct nfs4_deviceid_node *d;
spin_lock(&nfs4_deviceid_lock);
rcu_read_lock();
- d = _lookup_deviceid(clp, id, nfs4_deviceid_hash(id));
+ d = _lookup_deviceid(ld, clp, id, nfs4_deviceid_hash(id));
rcu_read_unlock();
if (!d) {
spin_unlock(&nfs4_deviceid_lock);
@@ -150,11 +155,12 @@ EXPORT_SYMBOL_GPL(nfs4_unhash_put_deviceid);
* @id deviceid to delete
*/
void
-nfs4_delete_deviceid(const struct nfs_client *clp, const struct nfs4_deviceid *id)
+nfs4_delete_deviceid(const struct pnfs_layoutdriver_type *ld,
+ const struct nfs_client *clp, const struct nfs4_deviceid *id)
{
struct nfs4_deviceid_node *d;
- d = nfs4_unhash_put_deviceid(clp, id);
+ d = nfs4_unhash_put_deviceid(ld, clp, id);
if (!d)
return;
if (d->ld->free_deviceid_node)
@@ -197,7 +203,7 @@ nfs4_insert_deviceid_node(struct nfs4_deviceid_node *new)
spin_lock(&nfs4_deviceid_lock);
hash = nfs4_deviceid_hash(&new->deviceid);
- d = _find_get_deviceid(new->nfs_client, &new->deviceid, hash);
+ d = _find_get_deviceid(new->ld, new->nfs_client, &new->deviceid, hash);
if (d) {
spin_unlock(&nfs4_deviceid_lock);
return d;
--
1.7.2.3
Cap BIO size it one page
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/nfs/objlayout/objio_osd.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
index bc5aa46..06bc032 100644
--- a/fs/nfs/objlayout/objio_osd.c
+++ b/fs/nfs/objlayout/objio_osd.c
@@ -561,6 +561,9 @@ static int _add_stripe_unit(struct objio_state *ios, unsigned *cur_pg,
unsigned bio_size = (ios->ol_state.nr_pages + pages_in_stripe) /
stripes;
+ if (BIO_MAX_PAGES_KMALLOC < bio_size)
+ bio_size = BIO_MAX_PAGES_KMALLOC;
+
per_dev->bio = bio_kmalloc(gfp_flags, bio_size);
if (unlikely(!per_dev->bio)) {
dprintk("Faild to allocate BIO size=%u\n", bio_size);
--
1.7.2.3
On 2011-05-24 18:08, Boaz Harrosh wrote:
> Take an extra reference on a device insert. So devices keep
> around in the cache until nfs_client release.
> (This was the behavior 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 accesing did->ref directly
This is the right time to do it, why wait?
>
> TODO: nfs4_insert_deviceid_node should check if there are too many
> devices and start purging them. Say by time from last use.
yeah, an lru list would be helpful to prune the lists and get rid
of unused devices. As we discussed before, this can be done
periodically, and all devices not used for a long enough interval can
be deleted.
Benny
>
> Signed-off-by: Boaz Harrosh <[email protected]>
> ---
> fs/nfs/objlayout/objio_osd.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
> index faacde2..8b05b16 100644
> --- a/fs/nfs/objlayout/objio_osd.c
> +++ b/fs/nfs/objlayout/objio_osd.c
> @@ -104,6 +104,7 @@ _dev_list_add(const struct nfs_server *nfss,
> de = n;
> }
>
> + atomic_inc(&de->id_node.ref);
> return de;
> }
>
On 05/24/2011 08:14 PM, Benny Halevy wrote:
> On 2011-05-24 18:07, Boaz Harrosh wrote:
>> Fix BUGs in the new "Use global-device-cache".
>>
>> One thing I don't understand is why the compiler did
>> not complain when the code was returning the wrong
>> type of structure
>>
>> Signed-off-by: Boaz Harrosh <[email protected]>
>> ---
>> fs/nfs/objlayout/objio_osd.c | 28 +++++++++++++++++++---------
>> 1 files changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
>> index 167cd1e..faacde2 100644
>> --- a/fs/nfs/objlayout/objio_osd.c
>> +++ b/fs/nfs/objlayout/objio_osd.c
>> @@ -56,6 +56,7 @@ objio_free_deviceid_node(struct nfs4_deviceid_node *d)
>> {
>> struct objio_dev_ent *de = container_of(d, struct objio_dev_ent, id_node);
>>
>> + dprintk("%s: free od=%p\n", __func__, de->od);
>> osduld_put_device(de->od);
>> kfree(de);
>> }
>> @@ -64,14 +65,18 @@ static struct objio_dev_ent *_dev_list_find(const struct nfs_server *nfss,
>> const struct nfs4_deviceid *d_id)
>> {
>> struct nfs4_deviceid_node *d;
>> + struct objio_dev_ent *de;
>>
>> d = nfs4_find_get_deviceid(nfss->pnfs_curr_ld, nfss->nfs_client, d_id);
>> if (!d)
>> return NULL;
>> - return container_of(d, struct objio_dev_ent, id_node);
>> +
>> + de = container_of(d, struct objio_dev_ent, id_node);
>> + return de;
>
> That's not really required as container_of() does the type casting
> for you.
>
Ye, I know that change is just a left over from a print between the
set and the return. (Else how could I debug this)
I than removed the print because these come a lot in a git clone for
example.
>> }
>>
>> -static int _dev_list_add(const struct nfs_server *nfss,
>> +static struct objio_dev_ent *
>> +_dev_list_add(const struct nfs_server *nfss,
>> const struct nfs4_deviceid *d_id, struct osd_dev *od,
>> gfp_t gfp_flags)
>> {
>> @@ -79,9 +84,12 @@ static int _dev_list_add(const struct nfs_server *nfss,
>> struct objio_dev_ent *de = kzalloc(sizeof(*de), gfp_flags);
>> struct objio_dev_ent *n;
>>
>> - if (!de)
>> - return -ENOMEM;
>> + if (!de) {
>> + dprintk("%s: -ENOMEM od=%p\n", __func__, od);
>> + return NULL;
>
> better return ERR_PTR(-ENOMEM)
> that will percolate up the stack via _device_lookup.
>
Right missed that one
> Thanks!
>
What thanks? beers on you next time! ;-)
> Benny
>
Boaz
>> + }
>>
>> + dprintk("%s: Adding od=%p\n", __func__, od);
>> nfs4_init_deviceid_node(&de->id_node,
>> nfss->pnfs_curr_ld,
>> nfss->nfs_client,
>> @@ -91,11 +99,12 @@ static int _dev_list_add(const struct nfs_server *nfss,
>> d = nfs4_insert_deviceid_node(&de->id_node);
>> n = container_of(d, struct objio_dev_ent, id_node);
>> if (n != de) {
>> - BUG_ON(n->od != od);
>> + dprintk("%s: Race with other n->od=%p\n", __func__, n->od);
>> objio_free_deviceid_node(&de->id_node);
>> + de = n;
>> }
>>
>> - return 0;
>> + return de;
>> }
>>
>> struct caps_buffers {
>> @@ -117,7 +126,7 @@ struct objio_segment {
>> unsigned comps_index;
>> unsigned num_comps;
>> /* variable length */
>> - struct objio_dev_ent *ods[0];
>> + struct objio_dev_ent *ods[];
>> };
>>
>> static inline struct objio_segment *
>> @@ -176,12 +185,13 @@ static struct objio_dev_ent *_device_lookup(struct pnfs_layout_hdr *pnfslay,
>> goto out;
>> }
>>
>> - _dev_list_add(NFS_SERVER(pnfslay->plh_inode), d_id, od, gfp_flags);
>> + ode = _dev_list_add(NFS_SERVER(pnfslay->plh_inode), d_id, od,
>> + gfp_flags);
>>
>> out:
>> dprintk("%s: return=%d\n", __func__, err);
>> objlayout_put_deviceinfo(deviceaddr);
>> - return err ? ERR_PTR(err) : od;
>> + return err ? ERR_PTR(err) : ode;
>> }
>>
>> static int objio_devices_lookup(struct pnfs_layout_hdr *pnfslay,
>
On 2011-05-24 18:04, Boaz Harrosh wrote:
> NULL deref on first ever call. (When device is not found)
>
> Signed-off-by: Boaz Harrosh <[email protected]>
> ---
> fs/nfs/pnfs_dev.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
> index 7997899..7e5542c 100644
> --- a/fs/nfs/pnfs_dev.c
> +++ b/fs/nfs/pnfs_dev.c
> @@ -100,7 +100,7 @@ _find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
>
> rcu_read_lock();
> d = _lookup_deviceid(ld, clp, id, hash);
> - if (!atomic_inc_not_zero(&d->ref))
> + if (!d || !atomic_inc_not_zero(&d->ref))
This makes more sense, no?
+ if (d && !atomic_inc_not_zero(&d->ref))
Benny
> d = NULL;
> rcu_read_unlock();
> return d;
On 05/24/2011 07:52 PM, Benny Halevy wrote:
> On 2011-05-24 18:04, Boaz Harrosh wrote:
>> NULL deref on first ever call. (When device is not found)
>>
>> Signed-off-by: Boaz Harrosh <[email protected]>
>> ---
>> fs/nfs/pnfs_dev.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
>> index 7997899..7e5542c 100644
>> --- a/fs/nfs/pnfs_dev.c
>> +++ b/fs/nfs/pnfs_dev.c
>> @@ -100,7 +100,7 @@ _find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
>>
>> rcu_read_lock();
>> d = _lookup_deviceid(ld, clp, id, hash);
>> - if (!atomic_inc_not_zero(&d->ref))
>> + if (!d || !atomic_inc_not_zero(&d->ref))
>
> This makes more sense, no?
> + if (d && !atomic_inc_not_zero(&d->ref))
>
> Benny
>
>> d = NULL;
Sure, since then d is already set to NULL, I guess
>> rcu_read_unlock();
>> return d;
>
Boaz
Wrong allocation and pointering in lseg_alloc.
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/nfs/objlayout/objio_osd.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
index a4201d8..167cd1e 100644
--- a/fs/nfs/objlayout/objio_osd.c
+++ b/fs/nfs/objlayout/objio_osd.c
@@ -117,7 +117,7 @@ struct objio_segment {
unsigned comps_index;
unsigned num_comps;
/* variable length */
- struct objio_dev_ent *ods[1];
+ struct objio_dev_ent *ods[0];
};
static inline struct objio_segment *
@@ -278,7 +278,6 @@ extern int objio_alloc_lseg(struct pnfs_layout_segment **outp,
struct pnfs_osd_layout layout;
struct pnfs_osd_object_cred *cur_comp, src_comp;
struct caps_buffers *caps_p;
-
int err;
err = pnfs_osd_xdr_decode_layout_map(&layout, &iter, xdr);
@@ -289,14 +288,16 @@ extern int objio_alloc_lseg(struct pnfs_layout_segment **outp,
if (unlikely(err))
return err;
- objio_seg = kzalloc(sizeof(*objio_seg) +
+ objio_seg = kzalloc(sizeof(*objio_seg) +
+ sizeof(objio_seg->ods[0]) * layout.olo_num_comps +
sizeof(*objio_seg->comps) * layout.olo_num_comps +
sizeof(struct caps_buffers) * layout.olo_num_comps,
gfp_flags);
if (!objio_seg)
return -ENOMEM;
- cur_comp = objio_seg->comps = (void *)(objio_seg + 1);
+ objio_seg->comps = (void *)(objio_seg->ods + layout.olo_num_comps);
+ cur_comp = objio_seg->comps;
caps_p = (void *)(cur_comp + layout.olo_num_comps);
while (pnfs_osd_xdr_decode_layout_comp(&src_comp, &iter, xdr, &err))
copy_single_comp(cur_comp++, &src_comp, caps_p++);
--
1.7.2.3
Fix BUGs in the new "Use global-device-cache".
One thing I don't understand is why the compiler did
not complain when the code was returning the wrong
type of structure
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/nfs/objlayout/objio_osd.c | 28 +++++++++++++++++++---------
1 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
index 167cd1e..faacde2 100644
--- a/fs/nfs/objlayout/objio_osd.c
+++ b/fs/nfs/objlayout/objio_osd.c
@@ -56,6 +56,7 @@ objio_free_deviceid_node(struct nfs4_deviceid_node *d)
{
struct objio_dev_ent *de = container_of(d, struct objio_dev_ent, id_node);
+ dprintk("%s: free od=%p\n", __func__, de->od);
osduld_put_device(de->od);
kfree(de);
}
@@ -64,14 +65,18 @@ static struct objio_dev_ent *_dev_list_find(const struct nfs_server *nfss,
const struct nfs4_deviceid *d_id)
{
struct nfs4_deviceid_node *d;
+ struct objio_dev_ent *de;
d = nfs4_find_get_deviceid(nfss->pnfs_curr_ld, nfss->nfs_client, d_id);
if (!d)
return NULL;
- return container_of(d, struct objio_dev_ent, id_node);
+
+ de = container_of(d, struct objio_dev_ent, id_node);
+ return de;
}
-static int _dev_list_add(const struct nfs_server *nfss,
+static struct objio_dev_ent *
+_dev_list_add(const struct nfs_server *nfss,
const struct nfs4_deviceid *d_id, struct osd_dev *od,
gfp_t gfp_flags)
{
@@ -79,9 +84,12 @@ static int _dev_list_add(const struct nfs_server *nfss,
struct objio_dev_ent *de = kzalloc(sizeof(*de), gfp_flags);
struct objio_dev_ent *n;
- if (!de)
- return -ENOMEM;
+ if (!de) {
+ dprintk("%s: -ENOMEM od=%p\n", __func__, od);
+ return NULL;
+ }
+ dprintk("%s: Adding od=%p\n", __func__, od);
nfs4_init_deviceid_node(&de->id_node,
nfss->pnfs_curr_ld,
nfss->nfs_client,
@@ -91,11 +99,12 @@ static int _dev_list_add(const struct nfs_server *nfss,
d = nfs4_insert_deviceid_node(&de->id_node);
n = container_of(d, struct objio_dev_ent, id_node);
if (n != de) {
- BUG_ON(n->od != od);
+ dprintk("%s: Race with other n->od=%p\n", __func__, n->od);
objio_free_deviceid_node(&de->id_node);
+ de = n;
}
- return 0;
+ return de;
}
struct caps_buffers {
@@ -117,7 +126,7 @@ struct objio_segment {
unsigned comps_index;
unsigned num_comps;
/* variable length */
- struct objio_dev_ent *ods[0];
+ struct objio_dev_ent *ods[];
};
static inline struct objio_segment *
@@ -176,12 +185,13 @@ static struct objio_dev_ent *_device_lookup(struct pnfs_layout_hdr *pnfslay,
goto out;
}
- _dev_list_add(NFS_SERVER(pnfslay->plh_inode), d_id, od, gfp_flags);
+ ode = _dev_list_add(NFS_SERVER(pnfslay->plh_inode), d_id, od,
+ gfp_flags);
out:
dprintk("%s: return=%d\n", __func__, err);
objlayout_put_deviceinfo(deviceaddr);
- return err ? ERR_PTR(err) : od;
+ return err ? ERR_PTR(err) : ode;
}
static int objio_devices_lookup(struct pnfs_layout_hdr *pnfslay,
--
1.7.2.3
the de-ref in pnfs_ld_read/write_done in the error case is not
needed. I only tested the write path but I suspect it is all
symetric
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/nfs/pnfs.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 0f59802..1716621 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -1069,8 +1069,6 @@ pnfs_ld_write_done(struct nfs_write_data *data)
return 0;
}
- put_lseg(data->lseg);
- data->lseg = NULL;
dprintk("%s: pnfs_error=%d, retry via MDS\n", __func__,
data->pnfs_error);
status = nfs_initiate_write(data, NFS_CLIENT(data->inode),
@@ -1118,8 +1116,6 @@ pnfs_ld_read_done(struct nfs_read_data *data)
return 0;
}
- put_lseg(data->lseg);
- data->lseg = NULL;
dprintk("%s: pnfs_error=%d, retry via MDS\n", __func__,
data->pnfs_error);
status = nfs_initiate_read(data, NFS_CLIENT(data->inode),
--
1.7.2.3
On 2011-05-24 18:05, Boaz Harrosh wrote:
> The atomic_dec_and_test() returns *true* when zero. The !
> belongs to atomic_dec(). Fixit
Thanks!
Benny
>
> Signed-off-by: Boaz Harrosh <[email protected]>
> ---
> fs/nfs/pnfs_dev.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
> index 7e5542c..37ca215 100644
> --- a/fs/nfs/pnfs_dev.c
> +++ b/fs/nfs/pnfs_dev.c
> @@ -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
On 2011-05-24 20:00, Boaz Harrosh wrote:
> On 05/24/2011 07:52 PM, Benny Halevy wrote:
>> On 2011-05-24 18:04, Boaz Harrosh wrote:
>>> NULL deref on first ever call. (When device is not found)
>>>
>>> Signed-off-by: Boaz Harrosh <[email protected]>
>>> ---
>>> fs/nfs/pnfs_dev.c | 2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
>>> index 7997899..7e5542c 100644
>>> --- a/fs/nfs/pnfs_dev.c
>>> +++ b/fs/nfs/pnfs_dev.c
>>> @@ -100,7 +100,7 @@ _find_get_deviceid(const struct pnfs_layoutdriver_type *ld,
>>>
>>> rcu_read_lock();
>>> d = _lookup_deviceid(ld, clp, id, hash);
>>> - if (!atomic_inc_not_zero(&d->ref))
>>> + if (!d || !atomic_inc_not_zero(&d->ref))
>>
>> This makes more sense, no?
>> + if (d && !atomic_inc_not_zero(&d->ref))
>>
>> Benny
>>
>>> d = NULL;
>
> Sure, since then d is already set to NULL, I guess
>
Right.
>>> rcu_read_unlock();
>>> return d;
>>
>
> Boaz
On 2011-05-24 18:05, Boaz Harrosh wrote:
> 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.
>
> Signed-off-by: Boaz Harrosh <[email protected]>
OK, we can do that.
Benny
> ---
> fs/nfs/pnfs_dev.c | 16 ++++------------
> 1 files changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
> index 37ca215..1b592d9 100644
> --- a/fs/nfs/pnfs_dev.c
> +++ b/fs/nfs/pnfs_dev.c
> @@ -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
On 2011-05-24 18:06, Boaz Harrosh wrote:
> Wrong allocation and pointering in lseg_alloc.
>
> Signed-off-by: Boaz Harrosh <[email protected]>
> ---
> fs/nfs/objlayout/objio_osd.c | 9 +++++----
> 1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
> index a4201d8..167cd1e 100644
> --- a/fs/nfs/objlayout/objio_osd.c
> +++ b/fs/nfs/objlayout/objio_osd.c
> @@ -117,7 +117,7 @@ struct objio_segment {
> unsigned comps_index;
> unsigned num_comps;
> /* variable length */
> - struct objio_dev_ent *ods[1];
> + struct objio_dev_ent *ods[0];
> };
>
> static inline struct objio_segment *
> @@ -278,7 +278,6 @@ extern int objio_alloc_lseg(struct pnfs_layout_segment **outp,
> struct pnfs_osd_layout layout;
> struct pnfs_osd_object_cred *cur_comp, src_comp;
> struct caps_buffers *caps_p;
> -
> int err;
>
> err = pnfs_osd_xdr_decode_layout_map(&layout, &iter, xdr);
> @@ -289,14 +288,16 @@ extern int objio_alloc_lseg(struct pnfs_layout_segment **outp,
> if (unlikely(err))
> return err;
>
> - objio_seg = kzalloc(sizeof(*objio_seg) +
> + objio_seg = kzalloc(sizeof(*objio_seg) +
nit: While at it, the trailing space is extraneous...
Benny
> + sizeof(objio_seg->ods[0]) * layout.olo_num_comps +
> sizeof(*objio_seg->comps) * layout.olo_num_comps +
> sizeof(struct caps_buffers) * layout.olo_num_comps,
> gfp_flags);
> if (!objio_seg)
> return -ENOMEM;
>
> - cur_comp = objio_seg->comps = (void *)(objio_seg + 1);
> + objio_seg->comps = (void *)(objio_seg->ods + layout.olo_num_comps);
> + cur_comp = objio_seg->comps;
> caps_p = (void *)(cur_comp + layout.olo_num_comps);
> while (pnfs_osd_xdr_decode_layout_comp(&src_comp, &iter, xdr, &err))
> copy_single_comp(cur_comp++, &src_comp, caps_p++);