Re: On https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=775c17386a6fd

On 5/28/26 10:42 PM, Dmitry Antipov wrote:
> On Wed, 2026-05-27 at 15:52 +0800, Joseph Qi wrote:
> 
>> ocfs2_validate_dx_root() returns -EROFS
>> ocfs2_read_blocks() goto read_failure → put_bh, bhs[0]=NULL, return -EROFS
>> ocfs2_read_block() returns -EROFS
>> ocfs2_read_dx_root() returns -EROFS
>> ocfs2_find_entry_dx() goto out, returns -EROFS
>>
>> On a second read of the same block, ocfs2_buffer_uptodate(ci, bh)
>> returns false (block was never added to the ocfs2 metadata cache because
>> the first read failed), so the block is re-read from disk and
>> re-validated — failing again.
>>
>> Am I missing something?
> 
> It seems that I've got the point. In ocfs2_read_blocks(), it's wrong
> to assume that non-NULL BH returned by sb_getblk() is exclusively
> owned by the caller and so put_bh() always drops b_count from 1 to 0.
> If it is not so, BH remains on hold and likely to be returned by the
> next call to sb_getblk() unchanged - that is, with BH_Uptodate bit set
> even if it has failed validation previously. So BH_Uptodate should be
> cleared immediately after validate() callback has detected some data
> inconsistency, regardless of what may happens next with that BH:
> 

It makes sense. Thanks to figure this out.
Could you please send a formal patch to fix this issue?

Thanks,
Joseph

> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
> index 701d27d908d4..6114299b121e 100644
> --- a/fs/ocfs2/buffer_head_io.c
> +++ b/fs/ocfs2/buffer_head_io.c
> @@ -350,8 +350,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr,
>  						wait_on_buffer(bh);
>  					put_bh(bh);
>  					bhs[i] = NULL;
> -				} else if (bh && buffer_uptodate(bh)) {
> -					clear_buffer_uptodate(bh);
>  				}
>  				continue;
>  			}
> @@ -380,8 +378,11 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr,
>  				BUG_ON(buffer_jbd(bh));
>  				clear_buffer_needs_validate(bh);
>  				status = validate(sb, bh);
> -				if (status)
> +				if (status) {
> +					if (buffer_uptodate(bh))
> +						clear_buffer_uptodate(bh);
>  					goto read_failure;
> +				}
>  			}
>  		}
>  
> Dmitry