From: Andy Adamson <[email protected]>
Do not get_lseg for a non-valid lseg.
Prepare for calling put_lseg outside of inode i_lock.
Signed-off-by: Andy Adamson <[email protected]>
---
fs/nfs/pnfs.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 6b2a95d..24620cf 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -845,7 +845,8 @@ pnfs_has_layout(struct pnfs_layout_hdr *lo,
list_for_each_entry(lseg, &lo->segs, fi_list) {
if (is_matching_lseg(lseg, range)) {
ret = lseg;
- get_lseg(ret);
+ if (lseg->valid)
+ get_lseg(ret);
break;
}
if (cmp_layout(range, &lseg->range) > 0)
@@ -889,7 +890,6 @@ pnfs_update_layout(struct inode *ino,
/* Check to see if the layout for the given range already exists */
lseg = pnfs_has_layout(lo, &arg);
if (lseg && !lseg->valid) {
- put_lseg_locked(lseg);
/* someone is cleaning the layout */
lseg = NULL;
goto out_unlock;
--
1.6.6
On 2010-10-07 15:37, [email protected] wrote:
> From: Andy Adamson <[email protected]>
>
> Do not get_lseg for a non-valid lseg.
> Prepare for calling put_lseg outside of inode i_lock.
>
> Signed-off-by: Andy Adamson <[email protected]>
> ---
> fs/nfs/pnfs.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index 6b2a95d..24620cf 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -845,7 +845,8 @@ pnfs_has_layout(struct pnfs_layout_hdr *lo,
> list_for_each_entry(lseg, &lo->segs, fi_list) {
> if (is_matching_lseg(lseg, range)) {
> ret = lseg;
> - get_lseg(ret);
> + if (lseg->valid)
> + get_lseg(ret);
We shouldn't be hiding this inside pnfs_has_layout
and have different side effects in the different cases.
Since we're called under the lock, pnfs_has_layout
can just return the lseg and never get a reference and its
caller can take it if necessary before releasing the lock.
Also, it doesn't need to be EXPORT_SYMBOL_GPLed as it's
not used outside of the nfs client module.
Benny
> break;
> }
> if (cmp_layout(range, &lseg->range) > 0)
> @@ -889,7 +890,6 @@ pnfs_update_layout(struct inode *ino,
> /* Check to see if the layout for the given range already exists */
> lseg = pnfs_has_layout(lo, &arg);
> if (lseg && !lseg->valid) {
> - put_lseg_locked(lseg);
> /* someone is cleaning the layout */
> lseg = NULL;
> goto out_unlock;
From: Andy Adamson <[email protected]>
The file layout free_lseg i/o operation called by destroy_lseg under the
inode->i_lock can call nfs_put_client() when a data
server is no longer referenced. nfs_put_client can end up taking the
i_mutex called in rpc_unlink (called from nfs_idmap_delete from
nfs_free_client) which can result in a deadlock.
Use a temporary list to hold layout segments to be freed, and free them outside
the inode->i_lock.
Reported-by: Fred Isaman <[email protected]>
Signed-off-by: Andy Adamson <[email protected]>
---
fs/nfs/pnfs.c | 53 ++++++++++++++++++++++++++---------------------------
fs/nfs/pnfs.h | 1 -
2 files changed, 26 insertions(+), 28 deletions(-)
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 24620cf..06fcc92 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -275,6 +275,7 @@ init_lseg(struct pnfs_layout_hdr *lo, struct pnfs_layout_segment *lseg)
lseg->layout = lo;
}
+/* Called without i_lock held */
static void
destroy_lseg(struct kref *kref)
{
@@ -285,29 +286,10 @@ destroy_lseg(struct kref *kref)
dprintk("--> %s\n", __func__);
NFS_SERVER(local->inode)->pnfs_curr_ld->free_lseg(lseg);
/* Matched by get_layout_hdr_locked in pnfs_insert_layout */
- put_layout_hdr_locked(local);
+ put_layout_hdr(local->inode);
}
void
-put_lseg_locked(struct pnfs_layout_segment *lseg)
-{
- bool do_wake_up;
- struct nfs_inode *nfsi;
-
- if (!lseg)
- return;
-
- dprintk("%s: lseg %p ref %d valid %d\n", __func__, lseg,
- atomic_read(&lseg->kref.refcount), lseg->valid);
- do_wake_up = !lseg->valid;
- nfsi = NFS_I(lseg->layout->inode);
- kref_put(&lseg->kref, destroy_lseg);
- if (do_wake_up)
- wake_up(&nfsi->lo_waitq);
-}
-EXPORT_SYMBOL_GPL(put_lseg_locked);
-
-void
put_lseg(struct pnfs_layout_segment *lseg)
{
bool do_wake_up;
@@ -320,9 +302,7 @@ put_lseg(struct pnfs_layout_segment *lseg)
atomic_read(&lseg->kref.refcount), lseg->valid);
do_wake_up = !lseg->valid;
nfsi = NFS_I(lseg->layout->inode);
- spin_lock(&nfsi->vfs_inode.i_lock);
kref_put(&lseg->kref, destroy_lseg);
- spin_unlock(&nfsi->vfs_inode.i_lock);
if (do_wake_up)
wake_up(&nfsi->lo_waitq);
}
@@ -354,10 +334,11 @@ _pnfs_can_return_lseg(struct pnfs_layout_segment *lseg)
}
static void
-pnfs_clear_lseg_list(struct pnfs_layout_hdr *lo,
+pnfs_clear_lseg_list(struct pnfs_layout_hdr *lo, struct list_head *tmp_list,
struct pnfs_layout_range *range)
{
struct pnfs_layout_segment *lseg, *next;
+
dprintk("%s:Begin lo %p offset %llu length %llu iomode %d\n",
__func__, lo, range->offset, range->length, range->iomode);
@@ -370,8 +351,7 @@ pnfs_clear_lseg_list(struct pnfs_layout_hdr *lo,
"offset %llu length %llu\n", __func__,
lseg, lseg->range.iomode, lseg->range.offset,
lseg->range.length);
- list_del(&lseg->fi_list);
- put_lseg_locked(lseg);
+ list_move(&lseg->fi_list, tmp_list);
}
if (list_empty(&lo->segs)) {
struct nfs_client *clp;
@@ -387,6 +367,21 @@ pnfs_clear_lseg_list(struct pnfs_layout_hdr *lo,
dprintk("%s:Return\n", __func__);
}
+static void
+pnfs_free_lseg_list(struct list_head *tmp_list)
+{
+ struct pnfs_layout_segment *lseg;
+
+ while (!list_empty(tmp_list)) {
+ lseg = list_entry(tmp_list->next, struct pnfs_layout_segment,
+ fi_list);
+ dprintk("%s calling put_lseg on %p\n", __func__, lseg);
+ list_del(&lseg->fi_list);
+ put_lseg(lseg);
+ }
+}
+
+
void
pnfs_layoutget_release(struct pnfs_layout_hdr *lo)
{
@@ -403,12 +398,14 @@ pnfs_layoutreturn_release(struct pnfs_layout_hdr *lo,
struct pnfs_layout_range *range)
{
struct nfs_inode *nfsi = NFS_I(lo->inode);
+ LIST_HEAD(tmp_list);
spin_lock(&nfsi->vfs_inode.i_lock);
if (range)
- pnfs_clear_lseg_list(lo, range);
+ pnfs_clear_lseg_list(lo, &tmp_list, range);
put_layout_hdr_locked(lo); /* Matched in _pnfs_return_layout */
spin_unlock(&nfsi->vfs_inode.i_lock);
+ pnfs_free_lseg_list(&tmp_list);
wake_up_all(&nfsi->lo_waitq);
}
@@ -421,11 +418,12 @@ pnfs_destroy_layout(struct nfs_inode *nfsi)
.offset = 0,
.length = NFS4_MAX_UINT64,
};
+ LIST_HEAD(tmp_list);
spin_lock(&nfsi->vfs_inode.i_lock);
lo = nfsi->layout;
if (lo) {
- pnfs_clear_lseg_list(lo, &range);
+ pnfs_clear_lseg_list(lo, &tmp_list, &range);
WARN_ON(!list_empty(&nfsi->layout->segs));
WARN_ON(!list_empty(&nfsi->layout->layouts));
WARN_ON(nfsi->layout->refcount != 1);
@@ -434,6 +432,7 @@ pnfs_destroy_layout(struct nfs_inode *nfsi)
put_layout_hdr_locked(lo);
}
spin_unlock(&nfsi->vfs_inode.i_lock);
+ pnfs_free_lseg_list(&tmp_list);
}
/*
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 1b1efcd..51f717d 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -177,7 +177,6 @@ extern int nfs4_proc_layoutreturn(struct nfs4_layoutreturn *lrp, bool wait);
/* pnfs.c */
void put_lseg(struct pnfs_layout_segment *lseg);
-void put_lseg_locked(struct pnfs_layout_segment *lseg);
struct pnfs_layout_segment *
pnfs_has_layout(struct pnfs_layout_hdr *lo, struct pnfs_layout_range *range);
struct pnfs_layout_segment *
--
1.6.6
pnfs_has_layout does not get_lref on its return value anymore
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfs/nfs4proc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 25bc169..97cc539 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5442,12 +5442,12 @@ nfs4_layoutget_prepare(struct rpc_task *task, void *calldata)
return;
}
if (!lseg->valid) {
- put_lseg_locked(lseg);
spin_unlock(&ino->i_lock);
dprintk("%s: invalid lseg found, waiting\n", __func__);
rpc_sleep_on(&nfsi->lo_rpcwaitq, task, NULL);
return;
}
+ get_lseg(lseg);
*lgp->lsegpp = lseg;
spin_unlock(&ino->i_lock);
dprintk("%s: valid lseg found, no rpc required\n", __func__);
--
1.7.2.3
On 2010-10-07 14:41, Benny Halevy wrote:
> On 2010-10-07 14:08, Benny Halevy wrote:
>> On 2010-10-07 13:06, Benny Halevy wrote:
>>> On 2010-10-07 15:37, [email protected] wrote:
>>>> From: Andy Adamson <[email protected]>
>>>>
>>>> Signed-off-by: Andy Adamson <[email protected]>
>>>> ---
>>>> fs/nfs/nfs4filelayoutdev.c | 5 -----
>>>> fs/nfs/nfs4state.c | 5 +++++
>>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>>>> index e0edf93..1f0ab62 100644
>>>> --- a/fs/nfs/nfs4filelayoutdev.c
>>>> +++ b/fs/nfs/nfs4filelayoutdev.c
>>>> @@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
>>>> goto out_put;
>>>> }
>>>> /*
>>>> - * Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
>>>> - * The is_ds_only_session depends on this.
>>>> - */
>>>> - clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
>>>> - /*
>>>> * Set DS lease equal to the MDS lease, renewal is scheduled in
>>>> * create_session
>>>> */
>>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>>>> index 91584ad..e2fc175 100644
>>>> --- a/fs/nfs/nfs4state.c
>>>> +++ b/fs/nfs/nfs4state.c
>>>> @@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
>>>> int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>>> {
>>>> int status;
>>>> + u32 req_exchange_flags = clp->cl_exchange_flags;
>>>>
>>>> nfs4_begin_drain_session(clp);
>>>> status = nfs4_proc_exchange_id(clp, cred);
>>>> @@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>>> status = nfs4_proc_create_session(clp);
>>>> if (status != 0)
>>>> goto out;
>>>> + if (is_ds_only_session(req_exchange_flags))
>>>> + /* Mask the (possibly) returned MDS and non-pNFS roles */
>>>
>>> This comment does not really add anything substantial that the code doesn't tell you :)
>>>
>>>> + clp->cl_exchange_flags &=
>>>> + ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
>>>
>>> I'm not why you want to mask out EXCHGID4_FLAG_USE_NON_PNFS.
>>> If the server is not a DS why not just return an error?
>>
>> So Andy convinced me and the spec. says that USE_PNFS_DS | USE_NON_PNFS
>> is a valid response.
>> However, if USE_PNFS_DS is unset in the response in this case
>> we need not create the client and better return an error.
>> I'll send a patch that does that.
>
> So this is what I have in mind:
>
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index e2fc175..4723c41 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -197,10 +197,14 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
> status = nfs4_proc_create_session(clp);
> if (status != 0)
> goto out;
> - if (is_ds_only_session(req_exchange_flags))
> - /* Mask the (possibly) returned MDS and non-pNFS roles */
> + if (is_ds_only_session(req_exchange_flags)) {
> clp->cl_exchange_flags &=
> ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
> + if (!is_ds_only_session(clp->cl_exchange_flags)) {
> + nfs4_proc_destroy_session(clp);
This should be {
nfs4_destroy_session(clp->cl_session);
clp->cl_session = NULL;
}
> + status = -ENOTSUPP;
> + }
> + }
> nfs41_setup_state_renewal(clp);
> nfs_mark_client_ready(clp, NFS_CS_READY);
> out:
>
>>
>> Benny
>>
>>>
>>> Benny
>>>
>>>> nfs41_setup_state_renewal(clp);
>>>> nfs_mark_client_ready(clp, NFS_CS_READY);
>>>> out:
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
On Thu, Oct 7, 2010 at 2:10 PM, Fred Isaman <[email protected]> wrote:
> On Thu, Oct 7, 2010 at 1:06 PM, Benny Halevy <[email protected]> wrote:
>> On 2010-10-07 15:37, [email protected] wrote:
>>> From: Andy Adamson <[email protected]>
>>>
>>> Signed-off-by: Andy Adamson <[email protected]>
>>> ---
>>> ?fs/nfs/nfs4filelayoutdev.c | ? ?5 -----
>>> ?fs/nfs/nfs4state.c ? ? ? ? | ? ?5 +++++
>>> ?2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>>> index e0edf93..1f0ab62 100644
>>> --- a/fs/nfs/nfs4filelayoutdev.c
>>> +++ b/fs/nfs/nfs4filelayoutdev.c
>>> @@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
>>> ? ? ? ? ? ? ? goto out_put;
>>> ? ? ? }
>>> ? ? ? /*
>>> - ? ? ?* Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
>>> - ? ? ?* The is_ds_only_session depends on this.
>>> - ? ? ?*/
>>> - ? ? clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
>>> - ? ? /*
>>> ? ? ? ?* Set DS lease equal to the MDS lease, renewal is scheduled in
>>> ? ? ? ?* create_session
>>> ? ? ? ?*/
>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>>> index 91584ad..e2fc175 100644
>>> --- a/fs/nfs/nfs4state.c
>>> +++ b/fs/nfs/nfs4state.c
>>> @@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
>>> ?int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>> ?{
>>> ? ? ? int status;
>>> + ? ? u32 req_exchange_flags = clp->cl_exchange_flags;
>>>
>>> ? ? ? nfs4_begin_drain_session(clp);
>>> ? ? ? status = nfs4_proc_exchange_id(clp, cred);
>>> @@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>> ? ? ? status = nfs4_proc_create_session(clp);
>>> ? ? ? if (status != 0)
>>> ? ? ? ? ? ? ? goto out;
>>> + ? ? if (is_ds_only_session(req_exchange_flags))
>>> + ? ? ? ? ? ? /* Mask the (possibly) returned MDS and non-pNFS roles */
>>
>> This comment does not really add anything substantial that the code doesn't tell you :)
>>
>>> + ? ? ? ? ? ? clp->cl_exchange_flags &=
>>> + ? ? ? ? ? ? ? ? ?~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
>>
>> I'm not why you want to mask out EXCHGID4_FLAG_USE_NON_PNFS.
>> If the server is not a DS why not just return an error?
>
> We *know* _USE_PNFS_DS is set. ?We just want to mask out
> _USE_NON_PNFS, which ?would indicate it is also a 4.0 server.
>
Oops...we *know* _PNFS_DS is set in req_exchange_flags. As you point
out, we would like to know it is in the reply.
Fred
> Fred
>
>>
>> Benny
>>
>>> ? ? ? nfs41_setup_state_renewal(clp);
>>> ? ? ? nfs_mark_client_ready(clp, NFS_CS_READY);
>>> ?out:
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>>
>
On Thu, Oct 7, 2010 at 2:52 PM, Benny Halevy <[email protected]> wrote:
> On 2010-10-07 15:37, [email protected] wrote:
>> From: Andy Adamson <[email protected]>
>>
>> Do not get_lseg for a non-valid lseg.
>> Prepare for calling put_lseg outside of inode i_lock.
>>
>> Signed-off-by: Andy Adamson <[email protected]>
>> ---
>> fs/nfs/pnfs.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index 6b2a95d..24620cf 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -845,7 +845,8 @@ pnfs_has_layout(struct pnfs_layout_hdr *lo,
>> list_for_each_entry(lseg, &lo->segs, fi_list) {
>> if (is_matching_lseg(lseg, range)) {
>> ret = lseg;
>> - get_lseg(ret);
>> + if (lseg->valid)
>> + get_lseg(ret);
>
> We shouldn't be hiding this inside pnfs_has_layout
> and have different side effects in the different cases.
Sorry - I just looked at the pnfs-submit branch. I should have looked
at the pnfs-all branch and seen the other callers of pnfs_has_layout.
> Since we're called under the lock, pnfs_has_layout
> can just return the lseg and never get a reference and its
> caller can take it if necessary before releasing the lock.
>
> Also, it doesn't need to be EXPORT_SYMBOL_GPLed as it's
> not used outside of the nfs client module.
Right - Fred just removed the call in the file layout driver.
I'll fix this.
-->Andy
>
> Benny
>
>> break;
>> }
>> if (cmp_layout(range, &lseg->range) > 0)
>> @@ -889,7 +890,6 @@ pnfs_update_layout(struct inode *ino,
>> /* Check to see if the layout for the given range already exists */
>> lseg = pnfs_has_layout(lo, &arg);
>> if (lseg && !lseg->valid) {
>> - put_lseg_locked(lseg);
>> /* someone is cleaning the layout */
>> lseg = NULL;
>> goto out_unlock;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 2010-10-07 14:08, Benny Halevy wrote:
> On 2010-10-07 13:06, Benny Halevy wrote:
>> On 2010-10-07 15:37, [email protected] wrote:
>>> From: Andy Adamson <[email protected]>
>>>
>>> Signed-off-by: Andy Adamson <[email protected]>
>>> ---
>>> fs/nfs/nfs4filelayoutdev.c | 5 -----
>>> fs/nfs/nfs4state.c | 5 +++++
>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>>> index e0edf93..1f0ab62 100644
>>> --- a/fs/nfs/nfs4filelayoutdev.c
>>> +++ b/fs/nfs/nfs4filelayoutdev.c
>>> @@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
>>> goto out_put;
>>> }
>>> /*
>>> - * Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
>>> - * The is_ds_only_session depends on this.
>>> - */
>>> - clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
>>> - /*
>>> * Set DS lease equal to the MDS lease, renewal is scheduled in
>>> * create_session
>>> */
>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>>> index 91584ad..e2fc175 100644
>>> --- a/fs/nfs/nfs4state.c
>>> +++ b/fs/nfs/nfs4state.c
>>> @@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
>>> int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>> {
>>> int status;
>>> + u32 req_exchange_flags = clp->cl_exchange_flags;
>>>
>>> nfs4_begin_drain_session(clp);
>>> status = nfs4_proc_exchange_id(clp, cred);
>>> @@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>> status = nfs4_proc_create_session(clp);
>>> if (status != 0)
>>> goto out;
>>> + if (is_ds_only_session(req_exchange_flags))
>>> + /* Mask the (possibly) returned MDS and non-pNFS roles */
>>
>> This comment does not really add anything substantial that the code doesn't tell you :)
>>
>>> + clp->cl_exchange_flags &=
>>> + ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
>>
>> I'm not why you want to mask out EXCHGID4_FLAG_USE_NON_PNFS.
>> If the server is not a DS why not just return an error?
>
> So Andy convinced me and the spec. says that USE_PNFS_DS | USE_NON_PNFS
> is a valid response.
> However, if USE_PNFS_DS is unset in the response in this case
> we need not create the client and better return an error.
> I'll send a patch that does that.
So this is what I have in mind:
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index e2fc175..4723c41 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -197,10 +197,14 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
status = nfs4_proc_create_session(clp);
if (status != 0)
goto out;
- if (is_ds_only_session(req_exchange_flags))
- /* Mask the (possibly) returned MDS and non-pNFS roles */
+ if (is_ds_only_session(req_exchange_flags)) {
clp->cl_exchange_flags &=
~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
+ if (!is_ds_only_session(clp->cl_exchange_flags)) {
+ nfs4_proc_destroy_session(clp);
+ status = -ENOTSUPP;
+ }
+ }
nfs41_setup_state_renewal(clp);
nfs_mark_client_ready(clp, NFS_CS_READY);
out:
>
> Benny
>
>>
>> Benny
>>
>>> nfs41_setup_state_renewal(clp);
>>> nfs_mark_client_ready(clp, NFS_CS_READY);
>>> out:
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Benny Halevy
Software Architect
Panasas, Inc.
[email protected]
Tel/Fax: +972-3-647-8340
Mobile: +972-54-802-8340
Panasas: The Leader in Parallel Storage
http://www.panasas.com
let its caller do that if necessary
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfs/pnfs.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 40d1dc6..a27df2b 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -844,8 +844,6 @@ pnfs_has_layout(struct pnfs_layout_hdr *lo,
list_for_each_entry(lseg, &lo->segs, fi_list) {
if (is_matching_lseg(lseg, range)) {
ret = lseg;
- if (lseg->valid)
- get_lseg(ret);
break;
}
if (cmp_layout(range, &lseg->range) > 0)
@@ -857,7 +855,6 @@ pnfs_has_layout(struct pnfs_layout_hdr *lo,
ret ? ret->valid : 0);
return ret;
}
-EXPORT_SYMBOL_GPL(pnfs_has_layout);
/*
* Layout segment is retreived from the server if not cached.
@@ -897,6 +894,7 @@ pnfs_update_layout(struct inode *ino,
if (lseg) {
dprintk("%s: Using cached lseg %p for iomode %d)\n",
__func__, lseg, iomode);
+ get_lseg(lseg);
goto out_unlock;
}
--
1.7.2.3
On 2010-10-07 15:37, [email protected] wrote:
> From: Andy Adamson <[email protected]>
>
> Signed-off-by: Andy Adamson <[email protected]>
> ---
> fs/nfs/nfs4filelayoutdev.c | 5 -----
> fs/nfs/nfs4state.c | 5 +++++
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
> index e0edf93..1f0ab62 100644
> --- a/fs/nfs/nfs4filelayoutdev.c
> +++ b/fs/nfs/nfs4filelayoutdev.c
> @@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
> goto out_put;
> }
> /*
> - * Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
> - * The is_ds_only_session depends on this.
> - */
> - clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
> - /*
> * Set DS lease equal to the MDS lease, renewal is scheduled in
> * create_session
> */
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 91584ad..e2fc175 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
> int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
> {
> int status;
> + u32 req_exchange_flags = clp->cl_exchange_flags;
>
> nfs4_begin_drain_session(clp);
> status = nfs4_proc_exchange_id(clp, cred);
> @@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
> status = nfs4_proc_create_session(clp);
> if (status != 0)
> goto out;
> + if (is_ds_only_session(req_exchange_flags))
> + /* Mask the (possibly) returned MDS and non-pNFS roles */
This comment does not really add anything substantial that the code doesn't tell you :)
> + clp->cl_exchange_flags &=
> + ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
I'm not why you want to mask out EXCHGID4_FLAG_USE_NON_PNFS.
If the server is not a DS why not just return an error?
Benny
> nfs41_setup_state_renewal(clp);
> nfs_mark_client_ready(clp, NFS_CS_READY);
> out:
On Thu, Oct 7, 2010 at 1:06 PM, Benny Halevy <[email protected]> wrote:
> On 2010-10-07 15:37, [email protected] wrote:
>> From: Andy Adamson <[email protected]>
>>
>> Signed-off-by: Andy Adamson <[email protected]>
>> ---
>> ?fs/nfs/nfs4filelayoutdev.c | ? ?5 -----
>> ?fs/nfs/nfs4state.c ? ? ? ? | ? ?5 +++++
>> ?2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>> index e0edf93..1f0ab62 100644
>> --- a/fs/nfs/nfs4filelayoutdev.c
>> +++ b/fs/nfs/nfs4filelayoutdev.c
>> @@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
>> ? ? ? ? ? ? ? goto out_put;
>> ? ? ? }
>> ? ? ? /*
>> - ? ? ?* Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
>> - ? ? ?* The is_ds_only_session depends on this.
>> - ? ? ?*/
>> - ? ? clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
>> - ? ? /*
>> ? ? ? ?* Set DS lease equal to the MDS lease, renewal is scheduled in
>> ? ? ? ?* create_session
>> ? ? ? ?*/
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index 91584ad..e2fc175 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
>> ?int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>> ?{
>> ? ? ? int status;
>> + ? ? u32 req_exchange_flags = clp->cl_exchange_flags;
>>
>> ? ? ? nfs4_begin_drain_session(clp);
>> ? ? ? status = nfs4_proc_exchange_id(clp, cred);
>> @@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>> ? ? ? status = nfs4_proc_create_session(clp);
>> ? ? ? if (status != 0)
>> ? ? ? ? ? ? ? goto out;
>> + ? ? if (is_ds_only_session(req_exchange_flags))
>> + ? ? ? ? ? ? /* Mask the (possibly) returned MDS and non-pNFS roles */
>
> This comment does not really add anything substantial that the code doesn't tell you :)
>
>> + ? ? ? ? ? ? clp->cl_exchange_flags &=
>> + ? ? ? ? ? ? ? ? ?~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
>
> I'm not why you want to mask out EXCHGID4_FLAG_USE_NON_PNFS.
> If the server is not a DS why not just return an error?
We *know* _USE_PNFS_DS is set. We just want to mask out
_USE_NON_PNFS, which would indicate it is also a 4.0 server.
Fred
>
> Benny
>
>> ? ? ? nfs41_setup_state_renewal(clp);
>> ? ? ? nfs_mark_client_ready(clp, NFS_CS_READY);
>> ?out:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
On 2010-10-07 13:06, Benny Halevy wrote:
> On 2010-10-07 15:37, [email protected] wrote:
>> From: Andy Adamson <[email protected]>
>>
>> Signed-off-by: Andy Adamson <[email protected]>
>> ---
>> fs/nfs/nfs4filelayoutdev.c | 5 -----
>> fs/nfs/nfs4state.c | 5 +++++
>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>> index e0edf93..1f0ab62 100644
>> --- a/fs/nfs/nfs4filelayoutdev.c
>> +++ b/fs/nfs/nfs4filelayoutdev.c
>> @@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
>> goto out_put;
>> }
>> /*
>> - * Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
>> - * The is_ds_only_session depends on this.
>> - */
>> - clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
>> - /*
>> * Set DS lease equal to the MDS lease, renewal is scheduled in
>> * create_session
>> */
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index 91584ad..e2fc175 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
>> int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>> {
>> int status;
>> + u32 req_exchange_flags = clp->cl_exchange_flags;
>>
>> nfs4_begin_drain_session(clp);
>> status = nfs4_proc_exchange_id(clp, cred);
>> @@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>> status = nfs4_proc_create_session(clp);
>> if (status != 0)
>> goto out;
>> + if (is_ds_only_session(req_exchange_flags))
>> + /* Mask the (possibly) returned MDS and non-pNFS roles */
>
> This comment does not really add anything substantial that the code doesn't tell you :)
>
>> + clp->cl_exchange_flags &=
>> + ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
>
> I'm not why you want to mask out EXCHGID4_FLAG_USE_NON_PNFS.
> If the server is not a DS why not just return an error?
So Andy convinced me and the spec. says that USE_PNFS_DS | USE_NON_PNFS
is a valid response.
However, if USE_PNFS_DS is unset in the response in this case
we need not create the client and better return an error.
I'll send a patch that does that.
Benny
>
> Benny
>
>> nfs41_setup_state_renewal(clp);
>> nfs_mark_client_ready(clp, NFS_CS_READY);
>> out:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Andy Adamson <[email protected]>
Signed-off-by: Andy Adamson <[email protected]>
---
fs/nfs/nfs4filelayoutdev.c | 5 -----
fs/nfs/nfs4state.c | 5 +++++
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
index e0edf93..1f0ab62 100644
--- a/fs/nfs/nfs4filelayoutdev.c
+++ b/fs/nfs/nfs4filelayoutdev.c
@@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
goto out_put;
}
/*
- * Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
- * The is_ds_only_session depends on this.
- */
- clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
- /*
* Set DS lease equal to the MDS lease, renewal is scheduled in
* create_session
*/
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 91584ad..e2fc175 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
{
int status;
+ u32 req_exchange_flags = clp->cl_exchange_flags;
nfs4_begin_drain_session(clp);
status = nfs4_proc_exchange_id(clp, cred);
@@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
status = nfs4_proc_create_session(clp);
if (status != 0)
goto out;
+ if (is_ds_only_session(req_exchange_flags))
+ /* Mask the (possibly) returned MDS and non-pNFS roles */
+ clp->cl_exchange_flags &=
+ ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
nfs41_setup_state_renewal(clp);
nfs_mark_client_ready(clp, NFS_CS_READY);
out:
--
1.6.6