Re: [bfields@xxxxxxxxxxxxxxxxxxxxx: all f08d6374 Merge branch 'nfs-for-next' into linux-next results]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sep 23, 2010, at 1:29 PM, Trond Myklebust wrote:

> On Thu, 2010-09-23 at 19:20 +0200, Benny Halevy wrote:
>> On 2010-09-21 23:58, Jeff Layton wrote:
>>> On Tue, 21 Sep 2010 16:46:21 -0400
>>> Trond Myklebust <trond.myklebust@xxxxxxxxxx> wrote:
>>> 
>>>> On Tue, 2010-09-21 at 16:24 -0400, J. Bruce Fields wrote:
>>>>> On Tue, Sep 21, 2010 at 04:00:32PM -0400, Trond Myklebust wrote:
>>>>>> On Tue, 2010-09-21 at 15:36 -0400, Trond Myklebust wrote:
>>>>>>> On Tue, 2010-09-21 at 15:20 -0400, J. Bruce Fields wrote:
>>>>>>>> All I need to do is mount nfs4.1 and run cthon -s.  The last output I
>>>>>>>> see from the test is:
>>>>>>>> 
>>>>>>>> check for proper open/unlink operation
>>>>>>>> nfsjunk files before unlink:
>>>>>>>> 
>>>>>>> 
>>>>>>> Oh... I bet I see what it is.
>>>>>>> 
>>>>>>> It's that damned res->sr_slotid = NFS4_MAX_SLOT_TABLE and related
>>>>>>> initialisation junk that's biting us in the arse again...
>>>>>>> 
>>>>>>> I'll fix it...
>>>>>> 
>>>>>> Does this work?
>>>>> 
>>>>> Yup.
>>>> 
>>>> It seems to fix things for me too... Once you mentioned NFSv4.1 it was
>>>> easy to reproduce the bug.
>>>> 
>>>> OK. I'll merge these into nfs-for-2.6.37...
>>> 
>>> Thanks for fixing it. I should have mentioned that the 4.1 parts were
>>> written using the "cargo-cult" programming method of copying what
>>> unlink does, and that they needed careful review. Mea culpa!
>>> 
>> 
>> I still hate it that the sr_slotid requires explicit, non-trivial initialization.
>> Once upon a time, the equivalent of sr_slotid used to be a pointer to
>> struct slot.  Using a pointer, implicitly initialized to NULL is significantly
>> more straight forward as in the patch below.
>> (passes cthon tests over your nfs-for-2.6.37 branch + the patch I
>> sent earlier that fixes __put_nfs_open_context. w/o it I readily reproduce the
>> crash reported on this thread)
>> 
>> From 50221fb5bd8fbd46f6cabe1fc795db7efc71bff7 Mon Sep 17 00:00:00 2001
>> From: Benny Halevy <bhalevy@xxxxxxxxxxx>
>> Date: Thu, 23 Sep 2010 19:08:21 +0200
>> Subject: [PATCH] NFSv4.1: keep seq_res.sr_slot as pointer rather than an index
>> 
>> Having to explicitly initialize sr_slotid to NFS4_MAX_SLOT_TABLE
>> resulted in numerous bugs.  Keeping the current slot as a pointer
>> to the slot table is more straight forward and robust as it's
>> implicitly set up to NULL wherever the seq_res member is initialized
>> to zeroes.
>> 
>> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
> 
> Looks fine to me, and I prefer this approach to the one we have today. 
> 
> Note, however, that I've already applied commit
> d688e11007419fd060ae74d8d952a5c4ece735aa (NFSv4.1: Fix the slotid
> initialisation in nfs_async_rename()) to the nfs-for-2.6.37 branch.
> 
> Are people happy with me rebasing nfs-for-2.6.37 to remove the above
> commit, and apply this one instead?

Yes, I like this approach as well and think it should be applied.

-->Andy

> 
> Cheers
>  Trond
> --
> 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

--
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


[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux