Re: [RFCv3 2/4] ext4: Add ext4_ilock & ext4_iunlock API

[Ritesh Harjani <riteshh@linux.ibm.com>]

Hello Matthew,

Thanks for the review.

On 11/20/19 4:53 PM, Matthew Bobrowski wrote:
> On Wed, Nov 20, 2019 at 10:30:22AM +0530, Ritesh Harjani wrote:
>> This adds ext4_ilock/iunlock types of APIs.
>> This is the preparation APIs to make shared
>> locking/unlocking & restarting with exclusive
>> locking/unlocking easier in next patch.
> 
> *scratches head*
> 
> A nit, but what's with the changelog wrapping at like ~40 characters?

Yup will fix that next time. Thanks.

> 
>> +#define EXT4_IOLOCK_EXCL	(1 << 0)
>> +#define EXT4_IOLOCK_SHARED	(1 << 1)
>>
>> +static inline void ext4_ilock(struct inode *inode, unsigned int iolock)
>> +{
>> +	if (iolock == EXT4_IOLOCK_EXCL)
>> +		inode_lock(inode);
>> +	else
>> +		inode_lock_shared(inode);
>> +}
>> +
>> +static inline void ext4_iunlock(struct inode *inode, unsigned int iolock)
>> +{
>> +	if (iolock == EXT4_IOLOCK_EXCL)
>> +		inode_unlock(inode);
>> +	else
>> +		inode_unlock_shared(inode);
>> +}
>> +
>> +static inline int ext4_ilock_nowait(struct inode *inode, unsigned int iolock)
>> +{
>> +	if (iolock == EXT4_IOLOCK_EXCL)
>> +		return inode_trylock(inode);
>> +	else
>> +		return inode_trylock_shared(inode);
>> +}
> 
> Is it really necessary for all these helpers to actually have the
> 'else' statement? Could we not just return/set whatever takes the
> 'else' branch directly from the end of these functions? I think it
> would be cleaner that way.

Sure np.

> 
> /me doesn't really like the naming of these functions either.

:) difference of opinion.

> 
> What's people's opinion on changing these for example:
>     - ext4_inode_lock()
>     - ext4_inode_unlock()
> 

ext4_ilock/iunlock sounds better to me as it is short too.
But if others have also have a strong opinion towards
ext4_inode_lock/unlock() - I am ok with that.


> Or, better yet, is there any reason why we've never actually
> considered naming such functions to have the verb precede the actual
> object that we're performing the operation on? In my opinion, it
> totally makes way more sense from a code readability standpoint and
> overall intent of the function. For example:
>     - ext4_lock_inode()
>     - ext4_unlock_inode()

Not against your suggestion here.
But in kernel I do see a preference towards object followed by a verb.
At least in vfs I see functions like inode_lock()/unlock().

Plus I would not deny that this naming is also inspired from
xfs_ilock()/iunlock API names.

> 
>> +static inline void ext4_ilock_demote(struct inode *inode, unsigned int iolock)
>> +{
>> +	BUG_ON(iolock != EXT4_IOLOCK_EXCL);
>> +	downgrade_write(&inode->i_rwsem);
>> +}
>> +
> 
> Same principle would also apply here.
> 
> On an ending note, I'm not really sure that I like the name of these
> macros. Like, for example, expand the macro 'EXT4_IOLOCK_EXCL' into
> plain english words as if you were reading it. This would translate to
> something like 'EXT4 INPUT/OUPUT LOCK EXCLUSIVE' or 'EXT4 IO LOCK
> EXCLUSIVE'. Just flipping the words around make a significant
> improvement for overall readability i.e. 'EXT4_EXCL_IOLOCK', which
> would expand out to 'EXT4 EXCLUSIVE IO LOCK'. Speaking of, is there

Ditto. Unless you and others have a strong objection, I would rather
keep this as is :)


> any reason why we don't mention 'INODE' here seeing as though that's
> the object we're actually protecting by taking one of these locking
> mechanisms?

hmm, it was increasing the name of the macro if I do it that way.
But that's ok. Is below macro name better?

#define EXT4_INODE_IOLOCK_EXCL		(1 << 0)
#define EXT4_INODE_IOLOCK_SHARED	(1 << 1)


Thanks for the review!!
-ritesh