Re: [PATCH v3 5/9] ext4: clarify return semantics of ext4_load_tail_bh()

[Jan Kara]

On Wed 08-07-26 14:20:45, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 
> ext4_load_tail_bh() returns NULL for both holes and clean unwritten
> buffers, but the conditions that lead to this are not obvious from the
> code alone. Document this behavior to clarify the return value, so that
> readers do not mistakenly assume that only holes result in a NULL
> return.
> 
> Also update the inline comment following the ext4_get_block() call to
> reflect this, and note that a lookup-only get_block (without
> EXT4_GET_BLOCKS_CREATE) never sets BH_Mapped for clean unwritten
> extents, which is why a clean unwritten bh falls through to the
> "nothing to do" path.
> 
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/ext4/inode.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index ba4fc3c93bb6..2cb1b93d3ed1 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4026,6 +4026,10 @@ void ext4_set_aops(struct inode *inode)
>   * because it might have data in pagecache (eg, if called from ext4_zero_range,
>   * ext4_punch_hole, etc) which needs to be properly zeroed out. Otherwise a
>   * racing writeback can come later and flush the stale pagecache to disk.
> + *
> + * Return the loaded bh if it actually needs zeroing - in written, dirty
> + * unwritten, or delalloc state. Return NULL if it's clean (i.e., a hole or
> + * a clean unwritten block).
>   */
>  static struct buffer_head *ext4_load_tail_bh(struct inode *inode, loff_t from)
>  {
> @@ -4067,7 +4071,12 @@ static struct buffer_head *ext4_load_tail_bh(struct inode *inode, loff_t from)
>  		err = ext4_get_block(inode, iblock, bh, 0);
>  		if (err < 0)
>  			goto unlock;
> -		/* unmapped? It's a hole - nothing to do */
> +		/*
> +		 * It's a hole or a clean unwritten block - nothing to do.
> +		 * Note that a lookup-only get_block (without
> +		 * EXT4_GET_BLOCKS_CREATE) never sets BH_Mapped for clean
> +		 * unwritten extents.
> +		 */
>  		if (!buffer_mapped(bh)) {
>  			BUFFER_TRACE(bh, "still unmapped");
>  			goto unlock;
> -- 
> 2.52.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR