Here are some pnfs fixes for 2.6.36 since the bakeathon:
[PATCH 1/1] SQUASHME: pnfsd-files: update layout stateid properly
[PATCH 1/1] pnfs: do not change layout stateid when dropping layouts.
[PATCH 2/3] pnfs: mark page with error in readpage_async_filler when crossing lsegs
[PATCH 3/3] SQUASHME: pnfs: filelayout: print_ds should use dprintk
On voluntary "forgets", where the client drops the layout on its own
the inode's layotu stateid must not change.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfs/nfs4xdr.c | 1 +
fs/nfs/pnfs.c | 6 ++++--
include/linux/nfs_xdr.h | 1 +
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 6d86633..31ccacf 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -5243,6 +5243,7 @@ static int decode_layoutreturn(struct xdr_stream *xdr,
p = xdr_inline_decode(xdr, 4);
if (unlikely(!p))
goto out_overflow;
+ res->valid = true;
res->lrs_present = be32_to_cpup(p);
if (res->lrs_present)
status = decode_stateid(xdr, &res->stateid);
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index ff1749a..8fa4887 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -593,9 +593,11 @@ pnfs_layoutreturn_release(struct nfs4_layoutreturn *lrp)
return;
spin_lock(&lrp->args.inode->i_lock);
pnfs_clear_lseg_list(lo, &tmp_list, &lrp->args.range);
- if (!lrp->res.lrs_present)
+ if (!lrp->res.valid)
+ ; /* forgetful model internal release */
+ else if (!lrp->res.lrs_present)
pnfs_invalidate_layout_stateid(lo);
- else
+ else
pnfs_set_layout_stateid(lo, &lrp->res.stateid);
put_layout_hdr_locked(lo); /* Matched in _pnfs_return_layout */
spin_unlock(&lrp->args.inode->i_lock);
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 80e6a36..37ff78a 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -269,6 +269,7 @@ struct nfs4_layoutreturn_args {
struct nfs4_layoutreturn_res {
struct nfs4_sequence_res seq_res;
+ bool valid; /* internal, true if received reply */
u32 lrs_present;
nfs4_stateid stateid;
};
--
1.7.2.3
On 2010-10-27 22:17, Fred Isaman wrote:
> On Wed, Oct 27, 2010 at 4:06 PM, Benny Halevy <[email protected]> wrote:
>> On 2010-10-27 21:49, Fred Isaman wrote:
>>> The change to printk was in response to Trond's complaint about
>>> successive dprintks.
>>>
>>> Instead, the following would work:
>>>
>>>
>>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>>> index 5f52e6f..2ce393c 100644
>>> --- a/fs/nfs/nfs4filelayout.c
>>> +++ b/fs/nfs/nfs4filelayout.c
>>> @@ -585,7 +585,8 @@ filelayout_commit(struct nfs_write_data *data, int sync)
>>> }
>>
>> If we're going this way, the ifdebug could cover the following
>> printout as well...
>>
>
> Did you mean preceding printout? By the way - the complaint about
Yeah, preceding the call to print_ds (following my comment :)
> successive dprintks was regarding
> print_ds_list repeatedly calling print_ds, which at the time used dprintk.
Why do we care to optimize the debug case so much?
print_ds_list is already calling print_ds inside ifdebug(FACILITY)
so the common, non-debug case is optimized correctly. I.e. we don't
repeatedly check the debug flag normally.
Benny
>
> Fred
>
>> Benny
>>
>>> dprintk("%s: Initiating commit: %llu USE DS:\n",
>>> __func__, file_offset);
>>> - print_ds(ds);
>>> + ifdebug(FACILITY)
>>> + print_ds(ds);
>>>
>>> /* Send COMMIT to data server */
>>> nfs_initiate_commit(dsdata, clnt, call_ops, sync);
>>>
>>>
>>> Fred
>>>
>>> On Wed, Oct 27, 2010 at 2:24 PM, Benny Halevy <[email protected]> wrote:
>>>> rather than printk to prevent printouts in non-debug mode
>>>> currently happening in filelayout_commit
>>>>
>>>> Signed-off-by: Benny Halevy <[email protected]>
>>>> ---
>>>> fs/nfs/nfs4filelayoutdev.c | 9 ++++-----
>>>> 1 files changed, 4 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>>>> index 1f0ab62..de47112 100644
>>>> --- a/fs/nfs/nfs4filelayoutdev.c
>>>> +++ b/fs/nfs/nfs4filelayoutdev.c
>>>> @@ -53,10 +53,10 @@ void
>>>> print_ds(struct nfs4_pnfs_ds *ds)
>>>> {
>>>> if (ds == NULL) {
>>>> - printk("%s NULL device\n", __func__);
>>>> + dprintk("%s NULL device\n", __func__);
>>>> return;
>>>> }
>>>> - printk(" ip_addr %x port %hu\n"
>>>> + dprintk(" ip_addr %x port %hu\n"
>>>> " ref count %d\n"
>>>> " client %p\n"
>>>> " cl_exchange_flags %x\n",
>>>> @@ -71,7 +71,7 @@ print_ds_list(struct nfs4_file_layout_dsaddr *dsaddr)
>>>> int i;
>>>>
>>>> ifdebug(FACILITY) {
>>>> - printk("%s dsaddr->ds_num %d\n", __func__,
>>>> + dprintk("%s dsaddr->ds_num %d\n", __func__,
>>>> dsaddr->ds_num);
>>>> for (i = 0; i < dsaddr->ds_num; i++)
>>>> print_ds(dsaddr->ds_list[i]);
>>>> @@ -211,8 +211,7 @@ static void
>>>> destroy_ds(struct nfs4_pnfs_ds *ds)
>>>> {
>>>> dprintk("--> %s\n", __func__);
>>>> - ifdebug(FACILITY)
>>>> - print_ds(ds);
>>>> + print_ds(ds);
>>>>
>>>> if (ds->ds_clp)
>>>> nfs_put_client(ds->ds_clp);
>>>> --
>>>> 1.7.2.3
>>>>
>>>> --
>>>> 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 Wed, 2010-10-27 at 23:00 +0200, Benny Halevy wrote:
> On 2010-10-27 22:17, Fred Isaman wrote:
> > On Wed, Oct 27, 2010 at 4:06 PM, Benny Halevy <[email protected]> wrote:
> >> On 2010-10-27 21:49, Fred Isaman wrote:
> >>> The change to printk was in response to Trond's complaint about
> >>> successive dprintks.
> >>>
> >>> Instead, the following would work:
> >>>
> >>>
> >>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
> >>> index 5f52e6f..2ce393c 100644
> >>> --- a/fs/nfs/nfs4filelayout.c
> >>> +++ b/fs/nfs/nfs4filelayout.c
> >>> @@ -585,7 +585,8 @@ filelayout_commit(struct nfs_write_data *data, int sync)
> >>> }
> >>
> >> If we're going this way, the ifdebug could cover the following
> >> printout as well...
> >>
> >
> > Did you mean preceding printout? By the way - the complaint about
>
> Yeah, preceding the call to print_ds (following my comment :)
>
> > successive dprintks was regarding
> > print_ds_list repeatedly calling print_ds, which at the time used dprintk.
>
> Why do we care to optimize the debug case so much?
> print_ds_list is already calling print_ds inside ifdebug(FACILITY)
> so the common, non-debug case is optimized correctly. I.e. we don't
> repeatedly check the debug flag normally.
It's not about optimizing the debug case. It's about avoiding having to
check ifdebug(FACILITY) all the time when we're _not_ debugging.
Trond
From: Benny Halevy <[email protected]>
Signed-off-by: Benny Halevy <[email protected]>
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfs/read.c | 20 ++++++++++----------
1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index e1e1a65..1df536a 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -605,24 +605,24 @@ readpage_async_filler(void *data, struct page *page)
{
struct nfs_readdesc *desc = (struct nfs_readdesc *)data;
struct inode *inode = page->mapping->host;
- struct pnfs_layout_range *range;
struct nfs_page *new;
unsigned int len;
- loff_t pgoff;
int error;
len = nfs_page_length(page);
if (len == 0)
return nfs_return_empty_page(page);
- pgoff = (loff_t)page->index << PAGE_CACHE_SHIFT;
- range = desc->pgio->pg_lseg ? &desc->pgio->pg_lseg->range : NULL;
- if (!range ||
- (range->offset > pgoff + len) ||
- (range->offset + range->length < pgoff)) {
- put_lseg(desc->pgio->pg_lseg);
- desc->pgio->pg_lseg = pnfs_update_layout(inode, desc->ctx,
- pgoff, len, IOMODE_READ);
+ if (desc->pgio->pg_lseg) {
+ loff_t pgoff = (loff_t)page->index << PAGE_CACHE_SHIFT;
+ struct pnfs_layout_range *range = &desc->pgio->pg_lseg->range;
+
+ /* retry later with the right lseg? */
+ if (range->offset > pgoff + len ||
+ range->offset + range->length < pgoff) {
+ new = ERR_PTR(-EAGAIN);
+ goto out_error;
+ }
}
new = nfs_create_request(desc->ctx, inode, page, 0, len,
--
1.7.2.3
On 2010-10-27 21:49, Fred Isaman wrote:
> The change to printk was in response to Trond's complaint about
> successive dprintks.
>
> Instead, the following would work:
>
>
> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
> index 5f52e6f..2ce393c 100644
> --- a/fs/nfs/nfs4filelayout.c
> +++ b/fs/nfs/nfs4filelayout.c
> @@ -585,7 +585,8 @@ filelayout_commit(struct nfs_write_data *data, int sync)
> }
If we're going this way, the ifdebug could cover the following
printout as well...
Benny
> dprintk("%s: Initiating commit: %llu USE DS:\n",
> __func__, file_offset);
> - print_ds(ds);
> + ifdebug(FACILITY)
> + print_ds(ds);
>
> /* Send COMMIT to data server */
> nfs_initiate_commit(dsdata, clnt, call_ops, sync);
>
>
> Fred
>
> On Wed, Oct 27, 2010 at 2:24 PM, Benny Halevy <[email protected]> wrote:
>> rather than printk to prevent printouts in non-debug mode
>> currently happening in filelayout_commit
>>
>> Signed-off-by: Benny Halevy <[email protected]>
>> ---
>> fs/nfs/nfs4filelayoutdev.c | 9 ++++-----
>> 1 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>> index 1f0ab62..de47112 100644
>> --- a/fs/nfs/nfs4filelayoutdev.c
>> +++ b/fs/nfs/nfs4filelayoutdev.c
>> @@ -53,10 +53,10 @@ void
>> print_ds(struct nfs4_pnfs_ds *ds)
>> {
>> if (ds == NULL) {
>> - printk("%s NULL device\n", __func__);
>> + dprintk("%s NULL device\n", __func__);
>> return;
>> }
>> - printk(" ip_addr %x port %hu\n"
>> + dprintk(" ip_addr %x port %hu\n"
>> " ref count %d\n"
>> " client %p\n"
>> " cl_exchange_flags %x\n",
>> @@ -71,7 +71,7 @@ print_ds_list(struct nfs4_file_layout_dsaddr *dsaddr)
>> int i;
>>
>> ifdebug(FACILITY) {
>> - printk("%s dsaddr->ds_num %d\n", __func__,
>> + dprintk("%s dsaddr->ds_num %d\n", __func__,
>> dsaddr->ds_num);
>> for (i = 0; i < dsaddr->ds_num; i++)
>> print_ds(dsaddr->ds_list[i]);
>> @@ -211,8 +211,7 @@ static void
>> destroy_ds(struct nfs4_pnfs_ds *ds)
>> {
>> dprintk("--> %s\n", __func__);
>> - ifdebug(FACILITY)
>> - print_ds(ds);
>> + print_ds(ds);
>>
>> if (ds->ds_clp)
>> nfs_put_client(ds->ds_clp);
>> --
>> 1.7.2.3
>>
>> --
>> 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
>>
rather than printk to prevent printouts in non-debug mode
currently happening in filelayout_commit
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfs/nfs4filelayoutdev.c | 9 ++++-----
1 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
index 1f0ab62..de47112 100644
--- a/fs/nfs/nfs4filelayoutdev.c
+++ b/fs/nfs/nfs4filelayoutdev.c
@@ -53,10 +53,10 @@ void
print_ds(struct nfs4_pnfs_ds *ds)
{
if (ds == NULL) {
- printk("%s NULL device\n", __func__);
+ dprintk("%s NULL device\n", __func__);
return;
}
- printk(" ip_addr %x port %hu\n"
+ dprintk(" ip_addr %x port %hu\n"
" ref count %d\n"
" client %p\n"
" cl_exchange_flags %x\n",
@@ -71,7 +71,7 @@ print_ds_list(struct nfs4_file_layout_dsaddr *dsaddr)
int i;
ifdebug(FACILITY) {
- printk("%s dsaddr->ds_num %d\n", __func__,
+ dprintk("%s dsaddr->ds_num %d\n", __func__,
dsaddr->ds_num);
for (i = 0; i < dsaddr->ds_num; i++)
print_ds(dsaddr->ds_list[i]);
@@ -211,8 +211,7 @@ static void
destroy_ds(struct nfs4_pnfs_ds *ds)
{
dprintk("--> %s\n", __func__);
- ifdebug(FACILITY)
- print_ds(ds);
+ print_ds(ds);
if (ds->ds_clp)
nfs_put_client(ds->ds_clp);
--
1.7.2.3
On 2010-10-27 23:23, Trond Myklebust wrote:
> On Wed, 2010-10-27 at 23:00 +0200, Benny Halevy wrote:
>> On 2010-10-27 22:17, Fred Isaman wrote:
>>> On Wed, Oct 27, 2010 at 4:06 PM, Benny Halevy <[email protected]> wrote:
>>>> On 2010-10-27 21:49, Fred Isaman wrote:
>>>>> The change to printk was in response to Trond's complaint about
>>>>> successive dprintks.
>>>>>
>>>>> Instead, the following would work:
>>>>>
>>>>>
>>>>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>>>>> index 5f52e6f..2ce393c 100644
>>>>> --- a/fs/nfs/nfs4filelayout.c
>>>>> +++ b/fs/nfs/nfs4filelayout.c
>>>>> @@ -585,7 +585,8 @@ filelayout_commit(struct nfs_write_data *data, int sync)
>>>>> }
>>>>
>>>> If we're going this way, the ifdebug could cover the following
>>>> printout as well...
>>>>
>>>
>>> Did you mean preceding printout? By the way - the complaint about
>>
>> Yeah, preceding the call to print_ds (following my comment :)
>>
>>> successive dprintks was regarding
>>> print_ds_list repeatedly calling print_ds, which at the time used dprintk.
>>
>> Why do we care to optimize the debug case so much?
>> print_ds_list is already calling print_ds inside ifdebug(FACILITY)
>> so the common, non-debug case is optimized correctly. I.e. we don't
>> repeatedly check the debug flag normally.
>
> It's not about optimizing the debug case. It's about avoiding having to
> check ifdebug(FACILITY) all the time when we're _not_ debugging.
Right, and so we do, as the whole loop in print_ds_list is enclosed
in ifdebug(FACILITY).
Benny
>
> Trond
>
On Wed, Oct 27, 2010 at 4:06 PM, Benny Halevy <[email protected]> wrote:
> On 2010-10-27 21:49, Fred Isaman wrote:
>> The change to printk was in response to Trond's complaint about
>> successive dprintks.
>>
>> Instead, the following would work:
>>
>>
>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>> index 5f52e6f..2ce393c 100644
>> --- a/fs/nfs/nfs4filelayout.c
>> +++ b/fs/nfs/nfs4filelayout.c
>> @@ -585,7 +585,8 @@ filelayout_commit(struct nfs_write_data *data, int sync)
>> ? ? ? ? ? ? ? }
>
> If we're going this way, the ifdebug could cover the following
> printout as well...
>
Did you mean preceding printout? By the way - the complaint about
successive dprintks was regarding
print_ds_list repeatedly calling print_ds, which at the time used dprintk.
Fred
> Benny
>
>> ? ? ? ? ? ? ? dprintk("%s: Initiating commit: %llu USE DS:\n",
>> ? ? ? ? ? ? ? ? ? ? ? __func__, file_offset);
>> - ? ? ? ? ? ? print_ds(ds);
>> + ? ? ? ? ? ? ifdebug(FACILITY)
>> + ? ? ? ? ? ? ? ? ? ? print_ds(ds);
>>
>> ? ? ? ? ? ? ? /* Send COMMIT to data server */
>> ? ? ? ? ? ? ? nfs_initiate_commit(dsdata, clnt, call_ops, sync);
>>
>>
>> Fred
>>
>> On Wed, Oct 27, 2010 at 2:24 PM, Benny Halevy <[email protected]> wrote:
>>> rather than printk to prevent printouts in non-debug mode
>>> currently happening in filelayout_commit
>>>
>>> Signed-off-by: Benny Halevy <[email protected]>
>>> ---
>>> ?fs/nfs/nfs4filelayoutdev.c | ? ?9 ++++-----
>>> ?1 files changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>>> index 1f0ab62..de47112 100644
>>> --- a/fs/nfs/nfs4filelayoutdev.c
>>> +++ b/fs/nfs/nfs4filelayoutdev.c
>>> @@ -53,10 +53,10 @@ void
>>> ?print_ds(struct nfs4_pnfs_ds *ds)
>>> ?{
>>> ? ? ? ?if (ds == NULL) {
>>> - ? ? ? ? ? ? ? printk("%s NULL device\n", __func__);
>>> + ? ? ? ? ? ? ? dprintk("%s NULL device\n", __func__);
>>> ? ? ? ? ? ? ? ?return;
>>> ? ? ? ?}
>>> - ? ? ? printk(" ? ? ? ?ip_addr %x port %hu\n"
>>> + ? ? ? dprintk(" ? ? ? ?ip_addr %x port %hu\n"
>>> ? ? ? ? ? ? ? ?" ? ? ? ?ref count %d\n"
>>> ? ? ? ? ? ? ? ?" ? ? ? ?client %p\n"
>>> ? ? ? ? ? ? ? ?" ? ? ? ?cl_exchange_flags %x\n",
>>> @@ -71,7 +71,7 @@ print_ds_list(struct nfs4_file_layout_dsaddr *dsaddr)
>>> ? ? ? ?int i;
>>>
>>> ? ? ? ?ifdebug(FACILITY) {
>>> - ? ? ? ? ? ? ? printk("%s dsaddr->ds_num %d\n", __func__,
>>> + ? ? ? ? ? ? ? dprintk("%s dsaddr->ds_num %d\n", __func__,
>>> ? ? ? ? ? ? ? ? ? ? ? dsaddr->ds_num);
>>> ? ? ? ? ? ? ? ?for (i = 0; i < dsaddr->ds_num; i++)
>>> ? ? ? ? ? ? ? ? ? ? ? ?print_ds(dsaddr->ds_list[i]);
>>> @@ -211,8 +211,7 @@ static void
>>> ?destroy_ds(struct nfs4_pnfs_ds *ds)
>>> ?{
>>> ? ? ? ?dprintk("--> %s\n", __func__);
>>> - ? ? ? ifdebug(FACILITY)
>>> - ? ? ? ? ? ? ? print_ds(ds);
>>> + ? ? ? print_ds(ds);
>>>
>>> ? ? ? ?if (ds->ds_clp)
>>> ? ? ? ? ? ? ? ?nfs_put_client(ds->ds_clp);
>>> --
>>> 1.7.2.3
>>>
>>> --
>>> 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
>
The change to printk was in response to Trond's complaint about
successive dprintks.
Instead, the following would work:
diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index 5f52e6f..2ce393c 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -585,7 +585,8 @@ filelayout_commit(struct nfs_write_data *data, int sync)
}
dprintk("%s: Initiating commit: %llu USE DS:\n",
__func__, file_offset);
- print_ds(ds);
+ ifdebug(FACILITY)
+ print_ds(ds);
/* Send COMMIT to data server */
nfs_initiate_commit(dsdata, clnt, call_ops, sync);
Fred
On Wed, Oct 27, 2010 at 2:24 PM, Benny Halevy <[email protected]> wrote:
> rather than printk to prevent printouts in non-debug mode
> currently happening in filelayout_commit
>
> Signed-off-by: Benny Halevy <[email protected]>
> ---
> ?fs/nfs/nfs4filelayoutdev.c | ? ?9 ++++-----
> ?1 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
> index 1f0ab62..de47112 100644
> --- a/fs/nfs/nfs4filelayoutdev.c
> +++ b/fs/nfs/nfs4filelayoutdev.c
> @@ -53,10 +53,10 @@ void
> ?print_ds(struct nfs4_pnfs_ds *ds)
> ?{
> ? ? ? ?if (ds == NULL) {
> - ? ? ? ? ? ? ? printk("%s NULL device\n", __func__);
> + ? ? ? ? ? ? ? dprintk("%s NULL device\n", __func__);
> ? ? ? ? ? ? ? ?return;
> ? ? ? ?}
> - ? ? ? printk(" ? ? ? ?ip_addr %x port %hu\n"
> + ? ? ? dprintk(" ? ? ? ?ip_addr %x port %hu\n"
> ? ? ? ? ? ? ? ?" ? ? ? ?ref count %d\n"
> ? ? ? ? ? ? ? ?" ? ? ? ?client %p\n"
> ? ? ? ? ? ? ? ?" ? ? ? ?cl_exchange_flags %x\n",
> @@ -71,7 +71,7 @@ print_ds_list(struct nfs4_file_layout_dsaddr *dsaddr)
> ? ? ? ?int i;
>
> ? ? ? ?ifdebug(FACILITY) {
> - ? ? ? ? ? ? ? printk("%s dsaddr->ds_num %d\n", __func__,
> + ? ? ? ? ? ? ? dprintk("%s dsaddr->ds_num %d\n", __func__,
> ? ? ? ? ? ? ? ? ? ? ? dsaddr->ds_num);
> ? ? ? ? ? ? ? ?for (i = 0; i < dsaddr->ds_num; i++)
> ? ? ? ? ? ? ? ? ? ? ? ?print_ds(dsaddr->ds_list[i]);
> @@ -211,8 +211,7 @@ static void
> ?destroy_ds(struct nfs4_pnfs_ds *ds)
> ?{
> ? ? ? ?dprintk("--> %s\n", __func__);
> - ? ? ? ifdebug(FACILITY)
> - ? ? ? ? ? ? ? print_ds(ds);
> + ? ? ? print_ds(ds);
>
> ? ? ? ?if (ds->ds_clp)
> ? ? ? ? ? ? ? ?nfs_put_client(ds->ds_clp);
> --
> 1.7.2.3
>
> --
> 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 Wed, 2010-10-27 at 23:29 +0200, Benny Halevy wrote:
> On 2010-10-27 23:23, Trond Myklebust wrote:
> > On Wed, 2010-10-27 at 23:00 +0200, Benny Halevy wrote:
> >> On 2010-10-27 22:17, Fred Isaman wrote:
> >>> On Wed, Oct 27, 2010 at 4:06 PM, Benny Halevy <[email protected]> wrote:
> >>>> On 2010-10-27 21:49, Fred Isaman wrote:
> >>>>> The change to printk was in response to Trond's complaint about
> >>>>> successive dprintks.
> >>>>>
> >>>>> Instead, the following would work:
> >>>>>
> >>>>>
> >>>>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
> >>>>> index 5f52e6f..2ce393c 100644
> >>>>> --- a/fs/nfs/nfs4filelayout.c
> >>>>> +++ b/fs/nfs/nfs4filelayout.c
> >>>>> @@ -585,7 +585,8 @@ filelayout_commit(struct nfs_write_data *data, int sync)
> >>>>> }
> >>>>
> >>>> If we're going this way, the ifdebug could cover the following
> >>>> printout as well...
> >>>>
> >>>
> >>> Did you mean preceding printout? By the way - the complaint about
> >>
> >> Yeah, preceding the call to print_ds (following my comment :)
> >>
> >>> successive dprintks was regarding
> >>> print_ds_list repeatedly calling print_ds, which at the time used dprintk.
> >>
> >> Why do we care to optimize the debug case so much?
> >> print_ds_list is already calling print_ds inside ifdebug(FACILITY)
> >> so the common, non-debug case is optimized correctly. I.e. we don't
> >> repeatedly check the debug flag normally.
> >
> > It's not about optimizing the debug case. It's about avoiding having to
> > check ifdebug(FACILITY) all the time when we're _not_ debugging.
>
> Right, and so we do, as the whole loop in print_ds_list is enclosed
> in ifdebug(FACILITY).
In that case, I agree: the whole thing can be converted to use ordinary
printks...
Trond
On 2010-10-27 23:32, Trond Myklebust wrote:
> On Wed, 2010-10-27 at 23:29 +0200, Benny Halevy wrote:
>> On 2010-10-27 23:23, Trond Myklebust wrote:
>>> On Wed, 2010-10-27 at 23:00 +0200, Benny Halevy wrote:
>>>> On 2010-10-27 22:17, Fred Isaman wrote:
>>>>> On Wed, Oct 27, 2010 at 4:06 PM, Benny Halevy <[email protected]> wrote:
>>>>>> On 2010-10-27 21:49, Fred Isaman wrote:
>>>>>>> The change to printk was in response to Trond's complaint about
>>>>>>> successive dprintks.
>>>>>>>
>>>>>>> Instead, the following would work:
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>>>>>>> index 5f52e6f..2ce393c 100644
>>>>>>> --- a/fs/nfs/nfs4filelayout.c
>>>>>>> +++ b/fs/nfs/nfs4filelayout.c
>>>>>>> @@ -585,7 +585,8 @@ filelayout_commit(struct nfs_write_data *data, int sync)
>>>>>>> }
>>>>>>
>>>>>> If we're going this way, the ifdebug could cover the following
>>>>>> printout as well...
>>>>>>
>>>>>
>>>>> Did you mean preceding printout? By the way - the complaint about
>>>>
>>>> Yeah, preceding the call to print_ds (following my comment :)
>>>>
>>>>> successive dprintks was regarding
>>>>> print_ds_list repeatedly calling print_ds, which at the time used dprintk.
>>>>
>>>> Why do we care to optimize the debug case so much?
>>>> print_ds_list is already calling print_ds inside ifdebug(FACILITY)
>>>> so the common, non-debug case is optimized correctly. I.e. we don't
>>>> repeatedly check the debug flag normally.
>>>
>>> It's not about optimizing the debug case. It's about avoiding having to
>>> check ifdebug(FACILITY) all the time when we're _not_ debugging.
>>
>> Right, and so we do, as the whole loop in print_ds_list is enclosed
>> in ifdebug(FACILITY).
>
> In that case, I agree: the whole thing can be converted to use ordinary
> printks...
The point is it has a singular caller outside of this loop for which
the caller needs to check ifdebug(FACILITY) before calling print_ds.
Not a big deal, but I think it'd be cleaner if print_ds will do a dprintk
to simplify its singular usage, while the cost of that will be extra tests
when called, possibly many times, in debug mode from print_ds_list.
Benny
>
> Trond
>
On 2010-10-27 23:42, Benny Halevy wrote:
> On 2010-10-27 23:32, Trond Myklebust wrote:
>> On Wed, 2010-10-27 at 23:29 +0200, Benny Halevy wrote:
>>> On 2010-10-27 23:23, Trond Myklebust wrote:
>>>> On Wed, 2010-10-27 at 23:00 +0200, Benny Halevy wrote:
>>>>> On 2010-10-27 22:17, Fred Isaman wrote:
>>>>>> On Wed, Oct 27, 2010 at 4:06 PM, Benny Halevy <[email protected]> wrote:
>>>>>>> On 2010-10-27 21:49, Fred Isaman wrote:
>>>>>>>> The change to printk was in response to Trond's complaint about
>>>>>>>> successive dprintks.
>>>>>>>>
>>>>>>>> Instead, the following would work:
>>>>>>>>
>>>>>>>>
>>>>>>>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>>>>>>>> index 5f52e6f..2ce393c 100644
>>>>>>>> --- a/fs/nfs/nfs4filelayout.c
>>>>>>>> +++ b/fs/nfs/nfs4filelayout.c
>>>>>>>> @@ -585,7 +585,8 @@ filelayout_commit(struct nfs_write_data *data, int sync)
>>>>>>>> }
>>>>>>>
>>>>>>> If we're going this way, the ifdebug could cover the following
>>>>>>> printout as well...
>>>>>>>
>>>>>>
>>>>>> Did you mean preceding printout? By the way - the complaint about
>>>>>
>>>>> Yeah, preceding the call to print_ds (following my comment :)
>>>>>
>>>>>> successive dprintks was regarding
>>>>>> print_ds_list repeatedly calling print_ds, which at the time used dprintk.
>>>>>
>>>>> Why do we care to optimize the debug case so much?
>>>>> print_ds_list is already calling print_ds inside ifdebug(FACILITY)
>>>>> so the common, non-debug case is optimized correctly. I.e. we don't
>>>>> repeatedly check the debug flag normally.
>>>>
>>>> It's not about optimizing the debug case. It's about avoiding having to
>>>> check ifdebug(FACILITY) all the time when we're _not_ debugging.
>>>
>>> Right, and so we do, as the whole loop in print_ds_list is enclosed
>>> in ifdebug(FACILITY).
>>
>> In that case, I agree: the whole thing can be converted to use ordinary
>> printks...
>
> The point is it has a singular caller outside of this loop for which
> the caller needs to check ifdebug(FACILITY) before calling print_ds.
> Not a big deal, but I think it'd be cleaner if print_ds will do a dprintk
> to simplify its singular usage, while the cost of that will be extra tests
> when called, possibly many times, in debug mode from print_ds_list.
Nevermind, I just merged Fred's patch instead.
No need to spend that much energy on cosmetics in this case :)
Benny
>
> Benny
>
>>
>> Trond
>>
> --
> 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: Benny Halevy <[email protected]>
the layout stateid must be updated by layout get only
on success upon changing the actual state, otherwise,
a parallel layout_recall will send the wrong stateid.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4pnfsd.c | 35 ++++++++++++++++++++++-------------
1 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/fs/nfsd/nfs4pnfsd.c b/fs/nfsd/nfs4pnfsd.c
index 9ce2bf2..fc7f4e5 100644
--- a/fs/nfsd/nfs4pnfsd.c
+++ b/fs/nfsd/nfs4pnfsd.c
@@ -302,7 +302,7 @@ nfs4_process_layout_stateid(struct nfs4_client *clp, struct nfs4_file *fp,
if (lsp && ((verify_stateid(fp, stateid)) == 0)) {
dprintk("%s parallel initial layout state\n",
__func__);
- goto update;
+ goto verified;
}
dprintk("%s ERROR bad opaque in stateid 1\n", __func__);
@@ -314,14 +314,9 @@ nfs4_process_layout_stateid(struct nfs4_client *clp, struct nfs4_file *fp,
dprintk("%s bad stateid 1\n", __func__);
goto out_put;
}
-update:
- update_stateid(&ls->ls_stateid);
- dprintk("%s Updated ls_stateid to %d on layoutstate %p\n",
- __func__, ls->ls_stateid.si_generation, ls);
}
+verified:
status = 0;
- /* Set the stateid to be encoded */
- memcpy(stateid, &ls->ls_stateid, sizeof(stateid_t));
/* Return the layout state if requested */
if (lsp) {
@@ -351,13 +346,21 @@ free_layout(struct nfs4_layout *lp)
kmem_cache_free(pnfs_layout_slab, lp);
}
+#define update_layout_stateid(ls, sid) { \
+ update_stateid(&(ls)->ls_stateid); \
+ dprintk("%s Updated ls_stateid to %d on layoutstate %p\n", \
+ __func__, (ls)->ls_stateid.si_generation, (ls)); \
+ memcpy((sid), &(ls)->ls_stateid, sizeof(stateid_t)); \
+}
+
static void
init_layout(struct nfs4_layout_state *ls,
struct nfs4_layout *lp,
struct nfs4_file *fp,
struct nfs4_client *clp,
struct svc_fh *current_fh,
- struct nfsd4_layout_seg *seg)
+ struct nfsd4_layout_seg *seg,
+ stateid_t *stateid)
{
dprintk("pNFS %s: ls %p lp %p clp %p fp %p ino %p\n", __func__,
ls, lp, clp, fp, fp->fi_inode);
@@ -369,6 +372,7 @@ init_layout(struct nfs4_layout_state *ls,
lp->lo_state = ls;
memcpy(&lp->lo_seg, seg, sizeof(lp->lo_seg));
spin_lock(&layout_lock);
+ update_layout_stateid(ls, stateid);
list_add_tail(&lp->lo_perstate, &ls->ls_layouts);
list_add_tail(&lp->lo_perclnt, &clp->cl_layouts);
list_add_tail(&lp->lo_perfile, &fp->fi_layouts);
@@ -717,7 +721,7 @@ nfs4_pnfs_get_layout(struct nfsd4_pnfs_layoutget *lgp,
goto out_freelayout;
/* Can't merge, so let's initialize this new layout */
- init_layout(ls, lp, fp, clp, lgp->lg_fhp, &res.lg_seg);
+ init_layout(ls, lp, fp, clp, lgp->lg_fhp, &res.lg_seg, &lgp->lg_sid);
out_unlock:
if (ls)
put_layout_state(ls);
@@ -773,7 +777,8 @@ out:
static int
pnfs_return_file_layouts(struct nfs4_client *clp, struct nfs4_file *fp,
- struct nfsd4_pnfs_layoutreturn *lrp)
+ struct nfsd4_pnfs_layoutreturn *lrp,
+ struct nfs4_layout_state *ls)
{
int layouts_found = 0;
struct nfs4_layout *lp, *nextlp;
@@ -799,6 +804,8 @@ pnfs_return_file_layouts(struct nfs4_client *clp, struct nfs4_file *fp,
destroy_layout(lp);
}
}
+ if (ls && layouts_found && lrp->lrs_present)
+ update_layout_stateid(ls, &lrp->lr_sid);
spin_unlock(&layout_lock);
return layouts_found;
@@ -838,6 +845,7 @@ int nfs4_pnfs_return_layout(struct super_block *sb, struct svc_fh *current_fh,
struct inode *ino = current_fh->fh_dentry->d_inode;
struct nfs4_file *fp = NULL;
struct nfs4_client *clp;
+ struct nfs4_layout_state *ls = NULL;
u64 ex_fsid = current_fh->fh_export->ex_fsid;
void *recall_cookie = NULL;
@@ -859,13 +867,12 @@ int nfs4_pnfs_return_layout(struct super_block *sb, struct svc_fh *current_fh,
/* Check the stateid */
dprintk("%s PROCESS LO_STATEID inode %p\n", __func__, ino);
- status = nfs4_process_layout_stateid(clp, fp, &lrp->lr_sid,
- NULL);
+ status = nfs4_process_layout_stateid(clp, fp, &lrp->lr_sid, &ls);
if (status)
goto out_put_file;
/* update layouts */
- layouts_found = pnfs_return_file_layouts(clp, fp, lrp);
+ layouts_found = pnfs_return_file_layouts(clp, fp, lrp, ls);
/* optimize for the all-empty case */
if (list_empty(&fp->fi_layouts))
recall_cookie = PNFS_LAST_LAYOUT_NO_RECALLS;
@@ -884,6 +891,8 @@ int nfs4_pnfs_return_layout(struct super_block *sb, struct svc_fh *current_fh,
out_put_file:
if (fp)
put_nfs4_file(fp);
+ if (ls)
+ put_layout_state(ls);
out:
nfs4_unlock_state();
--
1.7.2.3