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
