Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 20 Nov 2019 22:23:40 +1100
From:   Matthew Bobrowski <mbobrowski@mbobrowski.org>
To:     Ritesh Harjani <riteshh@linux.ibm.com>
Cc:     jack@suse.cz, tytso@mit.edu, linux-ext4@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
Subject: Re: [RFCv3 2/4] ext4: Add ext4_ilock & ext4_iunlock API
Message-ID: <20191120112339.GB30486@bobrowski>
References: <20191120050024.11161-1-riteshh@linux.ibm.com>
 <20191120050024.11161-3-riteshh@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191120050024.11161-3-riteshh@linux.ibm.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

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?

> +#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.

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

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

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()

> +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
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?

/M