Message-ID: <45A76006.5070504@in.ibm.com>
Date:	Fri, 12 Jan 2007 15:46:38 +0530
From:	Srinivasa Ds <srinivasa@...ibm.com>
To:	Andrew Morton <akpm@...l.org>
CC:	Eric Sandeen <sandeen@...hat.com>,
	Alasdair G Kergon <agk@...hat.com>,
	linux-kernel@...r.kernel.org, dm-devel@...hat.com,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH 2.6.19 5/5] fs: freeze_bdev with semaphore not mutex

Andrew Morton wrote:
> On Tue, 07 Nov 2006 16:45:07 -0600
> Eric Sandeen <sandeen@...hat.com> wrote:
>
>   
>> Andrew Morton wrote:
>>
>>     
>>>> --- linux-2.6.19-rc4.orig/fs/buffer.c	2006-11-07 17:06:20.000000000 +0000
>>>> +++ linux-2.6.19-rc4/fs/buffer.c	2006-11-07 17:26:04.000000000 +0000
>>>> @@ -188,7 +188,9 @@ struct super_block *freeze_bdev(struct b
>>>>  {
>>>>  	struct super_block *sb;
>>>>  
>>>> -	mutex_lock(&bdev->bd_mount_mutex);
>>>> +	if (down_trylock(&bdev->bd_mount_sem))
>>>> +		return -EBUSY;
>>>> +
>>>>         
>>> This is a functional change which isn't described in the changelog.  What's
>>> happening here?
>>>       
>> Only allow one bdev-freezer in at a time, rather than queueing them up?
>>
>>     
>
> You're asking me? ;)
>
> Guys, I'm going to park this patch pending a full description of what it
> does, a description of what the above hunk is doing 
Iam really sorry for delayed reply. Here is my full explanation on my 
patch.
Patch, that I have proposed replaces the mutex on bd_mount_mutex with 
the semaphore (bd_mount_sem). It was(bd_mount_sem) used to be a 
semaphore earlier and then Ingo's patch changed it to mutex. Hence we 
had the problem in device mapper commands. But now, the proposed patch 
allows two separate device mapper commands to suspend and resume the 
logical devices. Even though this explaination is from deveice mapper 
perspective, since semaphore(bd_mount_sem) existed earlier, proposed 
patch doesn't hurt XFS code which access the freeze_bdev().
As for as the above code is concerned,Iam using down_trylock() to allow 
only one process to freeze the filesystem at a time and hence stops the 
other process from queuing up.
> and pending an
> examination of whether we'd be better off changing the mutex-debugging code
> rather than switching to semaphores.
>
>   
Regarding this one,I agree with what Arjan has told.
" It's not used as a mutex. Sad but true. It's not so easy to say "just
shut up the debug code", because it's just not that easy: The interface
allows double "unlock", which is fine for semaphores for example. There
fundamentally is no "owner" for this case, and all the mutex concepts
assume that there is an owner. If the owner goes away, pointers to their
task struct for example are no longer valid (used by lockdep and the
other debugging parts). It's what makes the difference between a mutex
and a semaphore: a mutex has an owner and several semantics follow from
that. These semantics allow a more efficient implementation (no multiple
"owners" possible) but once you go away from that fundamental property,
soon we'll see "oh and it needs <this extra code> to cover the wider
semantics correctly.. and soon you have a semaphore again.

Let true semaphores be semaphores, and make all real mutexes mutexes.
But lets not make actual semaphores use mutex code... ".

So Iam reproposing my patch(taken against latest kernel) here. Please 
let me know your comments on this.

Signed-off-by: Srinivasa DS <srinivasa@...ibm.com>
Signed-off-by: Alasdair G Kergon <agk@...hat.com>
Cc: Ingo Molnar <mingo@...e.hu>
Cc: Eric Sandeen <sandeen@...deen.net>
Cc: dm-devel@...hat.com




View attachment "dm.fix" of type "text/plain" (2378 bytes)

Powered by blists - more mailing lists