Re: [PATCH 2/6] ext4: clarify return semantics of ext4_load_tail_bh()

[Jan Kara]

On Wed 01-07-26 22:20:05, yizhang089@xxxxxxxxx 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: both holes and clean unwritten buffers fall through to the
> "nothing to do" path.
> 
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> ---
>  fs/ext4/inode.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index c2c2d6ac7f3d..0b31fa873743 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).
>   */

Great that you're adding this comment because when I was reading previous
patch, I've spent like 10 minutes trying to figure that out :). But as far
as I'm reading the code, ext4_load_tail_bh() will return the unwritten bh
even if it is clean - map_bh() in _ext4_get_block() will set
buffer_mapped() even for unwritten extent. What am I missing? BTW, it also
seems to be ext4_load_tail_bh() could attempt to ext4_read_bh_lock() on
unwritten bh if things align wrongly...

								Honza

>  static struct buffer_head *ext4_load_tail_bh(struct inode *inode, loff_t from)
>  {
> @@ -4065,7 +4069,7 @@ static struct buffer_head *ext4_load_tail_bh(struct inode *inode, loff_t from)
>  	if (!buffer_mapped(bh)) {
>  		BUFFER_TRACE(bh, "unmapped");
>  		ext4_get_block(inode, iblock, bh, 0);
> -		/* unmapped? It's a hole - nothing to do */
> +		/* It's a hole or a clean unwritten block - nothing to do */
>  		if (!buffer_mapped(bh)) {
>  			BUFFER_TRACE(bh, "still unmapped");
>  			goto unlock;
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR