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

[Jeff Mahoney <jeffm@suse.com>]

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

On 8/6/13 5:10 PM, Jeff Mahoney wrote:
> On 8/6/13 4:46 PM, Jan Kara wrote:
>> On Mon 05-08-13 20:12: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.
>> ...
>>
>>> --- a/fs/reiserfs/reiserfs.h	2013-08-05 20:08:59.258412220 -0400
>>> +++ b/fs/reiserfs/reiserfs.h	2013-08-05 20:09:52.537601042 -0400
>>> @@ -630,8 +630,8 @@ static inline int __reiserfs_is_journal_
>>>   */
>>>  void reiserfs_write_lock(struct super_block *s);
>>>  void reiserfs_write_unlock(struct super_block *s);
>>> -int reiserfs_write_lock_once(struct super_block *s);
>>> -void reiserfs_write_unlock_once(struct super_block *s, int lock_depth);
>>> +int __must_check reiserfs_write_unlock_nested(struct super_block *s);
>>> +void reiserfs_write_lock_nested(struct super_block *s, int depth);
>>>  
>>>  #ifdef CONFIG_REISERFS_CHECK
>>>  void reiserfs_lock_check_recursive(struct super_block *s);
>>> @@ -666,45 +666,54 @@ static inline void reiserfs_lock_check_r
>>>   * - The journal lock
>>>   * - The inode mutex
>>>   */
>>> -static inline void reiserfs_mutex_lock_safe(struct mutex *m,
>>> -			       struct super_block *s)
>>> +
>>> +#define reiserfs_safe(sb, action)			\
>>> +do {							\
>>> +	struct super_block *__sb = (sb);		\
>>> +	int __depth;					\
>>> +	__depth = reiserfs_write_unlock_nested(__sb);	\
>>> +	(action);					\
>>> +	reiserfs_write_lock_nested(__sb, __depth);	\
>>> +} while(0)
>>> +
>>> +#define reiserfs_mutex_lock_safe(mtx, s) reiserfs_safe(s, mutex_lock(mtx))
>>> +#define reiserfs_mutex_lock_nested_safe(mtx, subclass, s) \
>>> +	reiserfs_safe(s, mutex_lock_nested(mtx, subclass))
>>> +#define reiserfs_down_read_safe(sem, s) reiserfs_safe(s, down_read(sem))
>>> +
>>> +/*
>>> + * When we schedule, we usually want to also release the write lock,
>>> + * according to the previous bkl based locking scheme of reiserfs.
>>> + */
>>> +static inline void reiserfs_cond_resched(struct super_block *s)
>>>  {
>>> -	reiserfs_lock_check_recursive(s);
>>> -	reiserfs_write_unlock(s);
>>> -	mutex_lock(m);
>>> -	reiserfs_write_lock(s);
>>> +	if (need_resched())
>>> +		reiserfs_safe(s, schedule());
>>>  }
>>>  
>>> -static inline void
>>> -reiserfs_mutex_lock_nested_safe(struct mutex *m, unsigned int subclass,
>>> -			       struct super_block *s)
>>> +static inline struct buffer_head *
>>> +reiserfs_safe_sb_bread(struct super_block *s, sector_t block)
>>>  {
>>> -	reiserfs_lock_check_recursive(s);
>>> -	reiserfs_write_unlock(s);
>>> -	mutex_lock_nested(m, subclass);
>>> -	reiserfs_write_lock(s);
>>> +	int depth;
>>> +	struct buffer_head *bh;
>>> +
>>> +	depth = reiserfs_write_unlock_nested(s);
>>> +	bh = sb_bread(s, block);
>>> +	reiserfs_write_lock_nested(s, depth);
>>> +
>>> +	return bh;
>>>  }
>>>
>>> +void reiserfs_safe_lock_buffer(struct buffer_head *bh);
>>> +
>>>  static inline void
>>> -reiserfs_down_read_safe(struct rw_semaphore *sem, struct super_block *s)
>>> +reiserfs_safe_wait_on_buffer(struct buffer_head *bh, struct super_block *s)
>>>  {
>>> -	reiserfs_lock_check_recursive(s);
>>> -	reiserfs_write_unlock(s);
>>> -	down_read(sem);
>>> -	reiserfs_write_lock(s);
>>> -}
>>> +	int depth;
>>>  
>>> -/*
>>> - * When we schedule, we usually want to also release the write lock,
>>> - * according to the previous bkl based locking scheme of reiserfs.
>>> - */
>>> -static inline void reiserfs_cond_resched(struct super_block *s)
>>> -{
>>> -	if (need_resched()) {
>>> -		reiserfs_write_unlock(s);
>>> -		schedule();
>>> -		reiserfs_write_lock(s);
>>> -	}
>>> +	depth = reiserfs_write_unlock_nested(s);
>>> +	__wait_on_buffer(bh);
>>> +	reiserfs_write_lock_nested(s, depth);
>>>  }
>>>  
>>>  struct fid;
>>> @@ -2463,6 +2472,15 @@ int reiserfs_commit_for_inode(struct ino
>>>  int reiserfs_inode_needs_commit(struct inode *);
>>>  void reiserfs_update_inode_transaction(struct inode *);
>>>  void reiserfs_wait_on_write_block(struct super_block *s);
>>> +static inline void reiserfs_safe_wait_on_write_block(struct super_block *s)
>>> +{
>>> +	int depth;
>>> +
>>> +	depth = reiserfs_write_unlock_nested(s);
>>> +	reiserfs_wait_on_write_block(s);
>>> +	reiserfs_write_lock_nested(s, depth);
>>> +}
>>> +
>>   I didn't find where the above 'safe' functions are called...
> 
> Whoa. WTF. Thanks for the review. These aren't the same patches as the
> ones in my repo. No idea why these got sent instead of the right ones!

Actually, they are. The end result is right but the reiserfs_safe()
macro was an idea I had and then decided against it. It shouldn't be in
the final version. I must've accidentally merged the removal in the
wrong patch.

-Jeff


-- 
Jeff Mahoney
SUSE Labs


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