Re: [patch v3 2/3] reiserfs: locking, handle nested locks properly

[Jeff Mahoney <jeffm@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 3817 bytes --]

On 8/7/13 4:25 PM, Jan Kara wrote:
> On Wed 07-08-13 12:35:17, Jeff Mahoney wrote:
>> The reiserfs write lock replaced the BKL and uses similar semantics.
>>
>> Frederic's locking code makes a distinction between when the lock is nested
>> and when it's being acquired/released, but I don't think that's the right
>> distinction to make.
>>
>> The right distinction is between the lock being released at end-of-use and
>> the lock being released for a schedule. The unlock should return the depth
>> and the lock should restore it, rather than the other way around as it is now.
>>
>> This patch implements that and adds a number of places where the lock
>> should be dropped.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>>
>>  fs/reiserfs/bitmap.c   |    5 +-
>>  fs/reiserfs/dir.c      |    7 +--
>>  fs/reiserfs/fix_node.c |   26 ++++++------
>>  fs/reiserfs/inode.c    |   77 ++++++++++++++++--------------------
>>  fs/reiserfs/ioctl.c    |    7 +--
>>  fs/reiserfs/journal.c  |  104 ++++++++++++++++++++++++++-----------------------
>>  fs/reiserfs/lock.c     |   43 ++++++++++----------
>>  fs/reiserfs/namei.c    |   24 +++--------
>>  fs/reiserfs/prints.c   |    5 +-
>>  fs/reiserfs/reiserfs.h |   36 +++++++++-------
>>  fs/reiserfs/resize.c   |   10 +++-
>>  fs/reiserfs/stree.c    |   16 +++----
>>  fs/reiserfs/super.c    |    5 --
>>  13 files changed, 188 insertions(+), 177 deletions(-)
>>
>   Just one smaller comment below, otherwise the patch looks good.
> 
>> --- a/fs/reiserfs/stree.c	2013-08-07 10:23:30.205643392 -0400
>> +++ b/fs/reiserfs/stree.c	2013-08-07 10:23:34.037585247 -0400
>> @@ -550,7 +550,8 @@ static bool search_by_key_reada(struct s
>>  		 */
>>  		if (!buffer_uptodate(bh[j])) {
>>  			if (!unlocked) {
>> -				reiserfs_write_unlock(s);
>> +				int depth = -1; /* avoiding __must_check */
>> +				depth = reiserfs_write_unlock_nested(s);
>>  				unlocked = true;
>>  			}
>>  			ll_rw_block(READA, 1, bh + j);
>   This is ugly. It shouldn't be too hard to either modify
> search_by_key_reada() to return lock depth or wrap more from the callsite
> of search_by_key_reada() into the function so that it always returns with
> write lock locked...

Ok, I'll return lock depth instead. Dropping the lock is an operation
that may happen but doesn't always happen. Pulling more in (or pushing
it out) won't affect the complexity there.

-Jeff

>> @@ -625,7 +626,7 @@ int search_by_key(struct super_block *sb
>>  	block_number = SB_ROOT_BLOCK(sb);
>>  	expected_level = -1;
>>  	while (1) {
>> -
>> +		int depth;
>>  #ifdef CONFIG_REISERFS_CHECK
>>  		if (!(++repeat_counter % 50000))
>>  			reiserfs_warning(sb, "PAP-5100",
>> @@ -646,25 +647,26 @@ int search_by_key(struct super_block *sb
>>  		if ((bh = last_element->pe_buffer =
>>  		     sb_getblk(sb, block_number))) {
>>  			bool unlocked = false;
>> -
>> +			depth = REISERFS_SB(sb)->lock_depth;
>>  			if (!buffer_uptodate(bh) && reada_count > 1)
>>  				/* may unlock the write lock */
>>  				unlocked = search_by_key_reada(sb, reada_bh,
>>  						    reada_blocks, reada_count);
>> +
>>  			/*
>>  			 * If we haven't already unlocked the write lock,
>>  			 * then we need to do that here before reading
>>  			 * the current block
>>  			 */
>>  			if (!buffer_uptodate(bh) && !unlocked) {
>> -				reiserfs_write_unlock(sb);
>> +				depth = reiserfs_write_unlock_nested(sb);
>>  				unlocked = true;
>>  			}
>>  			ll_rw_block(READ, 1, &bh);
>>  			wait_on_buffer(bh);
>>  
>>  			if (unlocked)
>> -				reiserfs_write_lock(sb);
>> +				reiserfs_write_lock_nested(sb, depth);
>>  			if (!buffer_uptodate(bh))
>>  				goto io_error;
>>  		} else {
> 
> 								Honza
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 841 bytes --]