* [PATCH] NFS: Fix comparison between DS address lists
@ 2012-02-03 20:45 Weston Andros Adamson
2012-02-03 21:34 ` Bryan Schumaker
0 siblings, 1 reply; 3+ messages in thread
From: Weston Andros Adamson @ 2012-02-03 20:45 UTC (permalink / raw)
To: Trond.Myklebust; +Cc: linux-nfs, Weston Andros Adamson
data_server_cache entries should only be treated as the same if the address
list hasn't changed.
A new entry will be made when an MDS changes an address list (as seen by
GETDEVINFO). The old entry will be freed once all references are gone.
Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
fs/nfs/nfs4filelayoutdev.c | 71 +++++++++++++++-----------------------------
1 files changed, 24 insertions(+), 47 deletions(-)
diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
index 6eb59b0..420e748 100644
--- a/fs/nfs/nfs4filelayoutdev.c
+++ b/fs/nfs/nfs4filelayoutdev.c
@@ -108,58 +108,40 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2)
return false;
}
-/*
- * Lookup DS by addresses. The first matching address returns true.
- * nfs4_ds_cache_lock is held
- */
-static struct nfs4_pnfs_ds *
-_data_server_lookup_locked(struct list_head *dsaddrs)
+bool
+_same_data_server_addrs_locked(const struct list_head *dsaddrs1,
+ const struct list_head *dsaddrs2)
{
- struct nfs4_pnfs_ds *ds;
struct nfs4_pnfs_ds_addr *da1, *da2;
- list_for_each_entry(da1, dsaddrs, da_node) {
- list_for_each_entry(ds, &nfs4_data_server_cache, ds_node) {
- list_for_each_entry(da2, &ds->ds_addrs, da_node) {
- if (same_sockaddr(
- (struct sockaddr *)&da1->da_addr,
- (struct sockaddr *)&da2->da_addr))
- return ds;
- }
- }
+ /* step through both lists, comparing as we go */
+ for (da1 = list_first_entry(dsaddrs1, typeof(*da1), da_node),
+ da2 = list_first_entry(dsaddrs2, typeof(*da2), da_node);
+ da1 != NULL && da2 != NULL;
+ da1 = list_entry(da1->da_node.next, typeof(*da1), da_node),
+ da2 = list_entry(da2->da_node.next, typeof(*da2), da_node)) {
+ if (!same_sockaddr((struct sockaddr *)&da1->da_addr,
+ (struct sockaddr *)&da2->da_addr))
+ return false;
}
- return NULL;
+ if (da1 == NULL && da2 == NULL)
+ return true;
+
+ return false;
}
/*
- * Compare two lists of addresses.
+ * Lookup DS by addresses. nfs4_ds_cache_lock is held
*/
-static bool
-_data_server_match_all_addrs_locked(struct list_head *dsaddrs1,
- struct list_head *dsaddrs2)
+static struct nfs4_pnfs_ds *
+_data_server_lookup_locked(const struct list_head *dsaddrs)
{
- struct nfs4_pnfs_ds_addr *da1, *da2;
- size_t count1 = 0,
- count2 = 0;
-
- list_for_each_entry(da1, dsaddrs1, da_node)
- count1++;
-
- list_for_each_entry(da2, dsaddrs2, da_node) {
- bool found = false;
- count2++;
- list_for_each_entry(da1, dsaddrs1, da_node) {
- if (same_sockaddr((struct sockaddr *)&da1->da_addr,
- (struct sockaddr *)&da2->da_addr)) {
- found = true;
- break;
- }
- }
- if (!found)
- return false;
- }
+ struct nfs4_pnfs_ds *ds;
- return (count1 == count2);
+ list_for_each_entry(ds, &nfs4_data_server_cache, ds_node)
+ if (_same_data_server_addrs_locked(&ds->ds_addrs, dsaddrs))
+ return ds;
+ return NULL;
}
/*
@@ -356,11 +338,6 @@ nfs4_pnfs_ds_add(struct list_head *dsaddrs, gfp_t gfp_flags)
dprintk("%s add new data server %s\n", __func__,
ds->ds_remotestr);
} else {
- if (!_data_server_match_all_addrs_locked(&tmp_ds->ds_addrs,
- dsaddrs)) {
- dprintk("%s: multipath address mismatch: %s != %s",
- __func__, tmp_ds->ds_remotestr, remotestr);
- }
kfree(remotestr);
kfree(ds);
atomic_inc(&tmp_ds->ds_count);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] NFS: Fix comparison between DS address lists
2012-02-03 20:45 [PATCH] NFS: Fix comparison between DS address lists Weston Andros Adamson
@ 2012-02-03 21:34 ` Bryan Schumaker
2012-02-03 21:46 ` Adamson, Dros
0 siblings, 1 reply; 3+ messages in thread
From: Bryan Schumaker @ 2012-02-03 21:34 UTC (permalink / raw)
To: Weston Andros Adamson; +Cc: Trond.Myklebust, linux-nfs
On 02/03/12 15:45, Weston Andros Adamson wrote:
> data_server_cache entries should only be treated as the same if the address
> list hasn't changed.
>
> A new entry will be made when an MDS changes an address list (as seen by
> GETDEVINFO). The old entry will be freed once all references are gone.
>
> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
> ---
> fs/nfs/nfs4filelayoutdev.c | 71 +++++++++++++++-----------------------------
> 1 files changed, 24 insertions(+), 47 deletions(-)
>
> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
> index 6eb59b0..420e748 100644
> --- a/fs/nfs/nfs4filelayoutdev.c
> +++ b/fs/nfs/nfs4filelayoutdev.c
> @@ -108,58 +108,40 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2)
> return false;
> }
>
> -/*
> - * Lookup DS by addresses. The first matching address returns true.
> - * nfs4_ds_cache_lock is held
> - */
> -static struct nfs4_pnfs_ds *
> -_data_server_lookup_locked(struct list_head *dsaddrs)
> +bool
> +_same_data_server_addrs_locked(const struct list_head *dsaddrs1,
> + const struct list_head *dsaddrs2)
> {
> - struct nfs4_pnfs_ds *ds;
> struct nfs4_pnfs_ds_addr *da1, *da2;
>
> - list_for_each_entry(da1, dsaddrs, da_node) {
> - list_for_each_entry(ds, &nfs4_data_server_cache, ds_node) {
> - list_for_each_entry(da2, &ds->ds_addrs, da_node) {
> - if (same_sockaddr(
> - (struct sockaddr *)&da1->da_addr,
> - (struct sockaddr *)&da2->da_addr))
> - return ds;
> - }
> - }
> + /* step through both lists, comparing as we go */
> + for (da1 = list_first_entry(dsaddrs1, typeof(*da1), da_node),
> + da2 = list_first_entry(dsaddrs2, typeof(*da2), da_node);
> + da1 != NULL && da2 != NULL;
> + da1 = list_entry(da1->da_node.next, typeof(*da1), da_node),
> + da2 = list_entry(da2->da_node.next, typeof(*da2), da_node)) {
> + if (!same_sockaddr((struct sockaddr *)&da1->da_addr,
> + (struct sockaddr *)&da2->da_addr))
> + return false;
> }
Does anybody else in the kernel ever need to walk two lists at the same time? Would this be better as a helper function in list.h?
> - return NULL;
> + if (da1 == NULL && da2 == NULL)
> + return true;
> +
> + return false;
> }
>
> /*
> - * Compare two lists of addresses.
> + * Lookup DS by addresses. nfs4_ds_cache_lock is held
> */
> -static bool
> -_data_server_match_all_addrs_locked(struct list_head *dsaddrs1,
> - struct list_head *dsaddrs2)
> +static struct nfs4_pnfs_ds *
> +_data_server_lookup_locked(const struct list_head *dsaddrs)
> {
> - struct nfs4_pnfs_ds_addr *da1, *da2;
> - size_t count1 = 0,
> - count2 = 0;
> -
> - list_for_each_entry(da1, dsaddrs1, da_node)
> - count1++;
> -
> - list_for_each_entry(da2, dsaddrs2, da_node) {
> - bool found = false;
> - count2++;
> - list_for_each_entry(da1, dsaddrs1, da_node) {
> - if (same_sockaddr((struct sockaddr *)&da1->da_addr,
> - (struct sockaddr *)&da2->da_addr)) {
> - found = true;
> - break;
> - }
> - }
> - if (!found)
> - return false;
> - }
> + struct nfs4_pnfs_ds *ds;
>
> - return (count1 == count2);
> + list_for_each_entry(ds, &nfs4_data_server_cache, ds_node)
> + if (_same_data_server_addrs_locked(&ds->ds_addrs, dsaddrs))
> + return ds;
> + return NULL;
> }
>
> /*
> @@ -356,11 +338,6 @@ nfs4_pnfs_ds_add(struct list_head *dsaddrs, gfp_t gfp_flags)
> dprintk("%s add new data server %s\n", __func__,
> ds->ds_remotestr);
> } else {
> - if (!_data_server_match_all_addrs_locked(&tmp_ds->ds_addrs,
> - dsaddrs)) {
> - dprintk("%s: multipath address mismatch: %s != %s",
> - __func__, tmp_ds->ds_remotestr, remotestr);
> - }
> kfree(remotestr);
> kfree(ds);
> atomic_inc(&tmp_ds->ds_count);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] NFS: Fix comparison between DS address lists
2012-02-03 21:34 ` Bryan Schumaker
@ 2012-02-03 21:46 ` Adamson, Dros
0 siblings, 0 replies; 3+ messages in thread
From: Adamson, Dros @ 2012-02-03 21:46 UTC (permalink / raw)
To: Schumaker, Bryan; +Cc: Myklebust, Trond, <linux-nfs@vger.kernel.org>
[-- Attachment #1: Type: text/plain, Size: 4630 bytes --]
On Feb 3, 2012, at 4:34 PM, Bryan Schumaker wrote:
> On 02/03/12 15:45, Weston Andros Adamson wrote:
>
>> data_server_cache entries should only be treated as the same if the address
>> list hasn't changed.
>>
>> A new entry will be made when an MDS changes an address list (as seen by
>> GETDEVINFO). The old entry will be freed once all references are gone.
>>
>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>> ---
>> fs/nfs/nfs4filelayoutdev.c | 71 +++++++++++++++-----------------------------
>> 1 files changed, 24 insertions(+), 47 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>> index 6eb59b0..420e748 100644
>> --- a/fs/nfs/nfs4filelayoutdev.c
>> +++ b/fs/nfs/nfs4filelayoutdev.c
>> @@ -108,58 +108,40 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2)
>> return false;
>> }
>>
>> -/*
>> - * Lookup DS by addresses. The first matching address returns true.
>> - * nfs4_ds_cache_lock is held
>> - */
>> -static struct nfs4_pnfs_ds *
>> -_data_server_lookup_locked(struct list_head *dsaddrs)
>> +bool
>> +_same_data_server_addrs_locked(const struct list_head *dsaddrs1,
>> + const struct list_head *dsaddrs2)
>> {
>> - struct nfs4_pnfs_ds *ds;
>> struct nfs4_pnfs_ds_addr *da1, *da2;
>>
>> - list_for_each_entry(da1, dsaddrs, da_node) {
>> - list_for_each_entry(ds, &nfs4_data_server_cache, ds_node) {
>> - list_for_each_entry(da2, &ds->ds_addrs, da_node) {
>> - if (same_sockaddr(
>> - (struct sockaddr *)&da1->da_addr,
>> - (struct sockaddr *)&da2->da_addr))
>> - return ds;
>> - }
>> - }
>> + /* step through both lists, comparing as we go */
>> + for (da1 = list_first_entry(dsaddrs1, typeof(*da1), da_node),
>> + da2 = list_first_entry(dsaddrs2, typeof(*da2), da_node);
>> + da1 != NULL && da2 != NULL;
>> + da1 = list_entry(da1->da_node.next, typeof(*da1), da_node),
>> + da2 = list_entry(da2->da_node.next, typeof(*da2), da_node)) {
>> + if (!same_sockaddr((struct sockaddr *)&da1->da_addr,
>> + (struct sockaddr *)&da2->da_addr))
>> + return false;
>> }
>
>
> Does anybody else in the kernel ever need to walk two lists at the same time? Would this be better as a helper function in list.h?
I'm game, but I can see problems with implementing this in list.h:
1) the for loop breaks once either list runs out of entries. this means that
callers have to check the values of the 'pos' nodes after calling the macro
(depending on what they are doing).
a way around this is to not use a macro and instead make a list_cmp_ordered()
function with a cmp callback. that probably wouldn't be acceptable for
list.h
2) will that mean i have to implement a similar function for hlist, etc?
I'm really not sure how often this would be used.
-dros
>
>> - return NULL;
>> + if (da1 == NULL && da2 == NULL)
>> + return true;
>> +
>> + return false;
>> }
>>
>> /*
>> - * Compare two lists of addresses.
>> + * Lookup DS by addresses. nfs4_ds_cache_lock is held
>> */
>> -static bool
>> -_data_server_match_all_addrs_locked(struct list_head *dsaddrs1,
>> - struct list_head *dsaddrs2)
>> +static struct nfs4_pnfs_ds *
>> +_data_server_lookup_locked(const struct list_head *dsaddrs)
>> {
>> - struct nfs4_pnfs_ds_addr *da1, *da2;
>> - size_t count1 = 0,
>> - count2 = 0;
>> -
>> - list_for_each_entry(da1, dsaddrs1, da_node)
>> - count1++;
>> -
>> - list_for_each_entry(da2, dsaddrs2, da_node) {
>> - bool found = false;
>> - count2++;
>> - list_for_each_entry(da1, dsaddrs1, da_node) {
>> - if (same_sockaddr((struct sockaddr *)&da1->da_addr,
>> - (struct sockaddr *)&da2->da_addr)) {
>> - found = true;
>> - break;
>> - }
>> - }
>> - if (!found)
>> - return false;
>> - }
>> + struct nfs4_pnfs_ds *ds;
>>
>> - return (count1 == count2);
>> + list_for_each_entry(ds, &nfs4_data_server_cache, ds_node)
>> + if (_same_data_server_addrs_locked(&ds->ds_addrs, dsaddrs))
>> + return ds;
>> + return NULL;
>> }
>>
>> /*
>> @@ -356,11 +338,6 @@ nfs4_pnfs_ds_add(struct list_head *dsaddrs, gfp_t gfp_flags)
>> dprintk("%s add new data server %s\n", __func__,
>> ds->ds_remotestr);
>> } else {
>> - if (!_data_server_match_all_addrs_locked(&tmp_ds->ds_addrs,
>> - dsaddrs)) {
>> - dprintk("%s: multipath address mismatch: %s != %s",
>> - __func__, tmp_ds->ds_remotestr, remotestr);
>> - }
>> kfree(remotestr);
>> kfree(ds);
>> atomic_inc(&tmp_ds->ds_count);
>
>
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-02-03 21:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-03 20:45 [PATCH] NFS: Fix comparison between DS address lists Weston Andros Adamson
2012-02-03 21:34 ` Bryan Schumaker
2012-02-03 21:46 ` Adamson, Dros
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox