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@xxxxxxxxxx>
> ---
> 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);
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
