linux-ext4 - Re: [PATCH 2/3] ext4: Clear the unwritten buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 14 May 2009 11:10:02 +0530
From:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
To:	Eric Sandeen <sandeen@...hat.com>
Cc:	Theodore Tso <tytso@....edu>, cmm@...ibm.com,
	linux-ext4@...r.kernel.org
Subject: Re: [PATCH 2/3] ext4: Clear the unwritten buffer_head flag properly

On Wed, May 13, 2009 at 01:56:56PM -0500, Eric Sandeen wrote:
> > 
> > I think it is good. But one thing missing in the commit message is,
> > what happens if we do a write to prealloc space. Since a
> > get_block(create = 1) is now split into __get_block(create = 0 )  and
> > __get_block(create = 1). That would mean if we pass a buffer head with
> > BH_Unwritten cleared we will have
> > 
> > 
> > 1) buffer_head as BH_Unwritten cleared.
> > 
> > 2) __get_block(create = 0 ) -> Since it is prealloc space we will have
> > BH_Unwritten set .
> 
> Why do we need to set BH_Unwritten on a !create call at all?
> 
> Or maybe another way of asking is, are there any !create callers of
> get_block who -want- BH_Unwritten set?
> 
> Which is to say, should we just not be setting BH_Unwritten in get_block
> in the !create case, ever?

It should only be set in the !create case. With create == 1, we would
have already converted the uninitialized extent to initialized one and
the buffer_head should not be unwritten at all. My understanding is
unwritten flag is used to indicate the buffer_head state between a
write_begin and write_page phase with delayed allocation. ie, when we
write to fallocate space, since we have delalloc enabled, we  just
do a block lookup (get_block with create = 0). The buffer_head returned
in the above case should have unwritten set so that during writepage
we do the actual block allocation (get_block writh create = 1)
looking at the flag.


> 
> The comment:
> 
>  	/*
> +	 * The above get_blocks can cause the buffer to be
> +	 * marked unwritten. So clear the same.
> +	 */
> +	clear_buffer_unwritten(bh);
> 
> is imho not helpful; to me it says "we -just- set this, so clear it!"
> It leaves me scratching my head.
> 
> > 3) __get_block(create = 1) -> get the blocks out of prealloc space.
> > and retun with BH_Mapped set. 
> > 
> > That would imply we have BH_Unwritten and BH_Mapped set in the above
> > case which is wrong. So we need a BH_Unwritten clear between (2) and
> > (3). The patch does the same. May be we need to capture it in commit
> > message.
> 
> Better in comments, I think.  :)
> 

-aneesh
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html