[Ocfs2-devel] [PATCH 1/2] ocfs2: Zero the tail cluster when extending past i_size v2

[Tao Ma <tao.ma@oracle.com>]

Hi Joel,

On 07/06/2010 03:17 PM, Joel Becker wrote:
> On Mon, Jul 05, 2010 at 11:51:44AM +0800, Tao Ma wrote:
>>> +	/*
>>> +	 * If tail_blkno is in the cluster past i_size, we don't need
>>> +	 * to touch the cluster containing i_size at all.
>>> +	 */
>>> +	tail_cpos = i_size_read(inode)>>   osb->s_clustersize_bits;
>>> +	if (ocfs2_blocks_to_clusters(inode->i_sb, tail_blkno)>   tail_cpos)
>>> +		tail_cpos = ocfs2_blocks_to_clusters(inode->i_sb,
>>> +						     tail_blkno);
>> Can we always set tail_cpos in one line?
>> 	tail_cpos = ocfs2_blocks_to_clusters(inode->i_sb, tail_blkno)?
>> tail_cpos is either the same cluster as i_size or the next cluster
>> and both works for tail_blkno I guess?
>
> 	I had the same thought on Friday, but the current version passes
> testing and I was wary of changing that.
ok, so as you wish.
>
>>> +	/* Is there a cluster to zero? */
>>> +	if (!p_cpos)
>>> +		goto out;
>> For unwritten extent, we also need to clear the pages? If yes, the
>> solution doesn't complete if we have 2 unwritten extent, one
>> contains i_size while one passes i_size. Here we only clear the
>> pages for the 1st unwritten extent and leave the 2nd one untouched.
>
> 	We probably don't need to zero unwritten extents.  We cannot
> have an extent past i_size, can we?
we can. AFAICS, ocfs2_change_file_space will allocate unwritten extents 
and does't change i_size.
>
>>  From here to the call of CoW is a bit hard to understand. In 'if',
>> num_clusters is set for CoW and in 'else', blocks_to_zero is set. So
>> it isn't easy for the reader to tell why these 2 clauses are setting
>> different values. So how about my code below? It looks more
>> straightforward I think.
>>> +	if ((tail_cpos + num_clusters)>   pos_cpos) {
>>> +		num_clusters = pos_cpos - tail_cpos;
>>> +		if (pos_blkno>
>>> +		    ocfs2_clusters_to_blocks(inode->i_sb, pos_cpos))
>>> +			num_clusters += 1;
>>> +	} else {
>>> +		blocks_to_zero =
>>> +			ocfs2_clusters_to_blocks(inode->i_sb,
>>> +						 tail_cpos + num_clusters);
>>> +		blocks_to_zero -= tail_blkno;
>>> +	}
>>> +
>>> +	/* Now CoW the clusters we're about to zero */
>>> +	if (ext_flags&   OCFS2_EXT_REFCOUNTED) {
>>> +		rc = ocfs2_refcount_cow(inode, di_bh, tail_cpos,
>>> +					num_clusters, UINT_MAX);
>>> +		if (rc) {
>>> +			mlog_errno(rc);
>>> +			goto out;
>>> +		}
>>> +	}
>> 	/* Decrease blocks_to_zero if there is some hole after extent */
>> 	if (tail_cpos + num_clusters<= pos_cpos) {
>> 		blocks_to_zero =
>> 			ocfs2_clusters_to_blocks(inode->i_sb,
>> 						 tail_cpos + num_clusters);
>> 		blocks_to_zero -= tail_blkno;
>> 	}
>
> 	Not a bad split-out here.
>
>> 	/* Now CoW if we have some refcounted clusters. */
>> 	if (ext_flags&  OCFS2_EXT_REFCOUNTED) {
>> 		/*
>> 		 * We add one more cluster here since it will be
>> 		 * written shortly and if the pos_blkno isn't aligned
>> 		 * to the cluster size, we have to zero the blocks
>> 		 * before it.
>> 		 */
>> 		if (tail_cpos + num_clusters>  pos_cpos)
>> 			num_clusters = pos_cpos - tail_cpos + 1;
>
> 	But you dropped the check for pos_blkno alignment.
> Unconditionally adding the +1 doesn't seem like a good idea.
You can add it as you wish.
I just thought that you add one more extra cluster if pos_blkno isn't 
aligned so as to zero blocks in [pos_cpos_start_block, pos_blkno).
But As I said in the comments, you will soon write pos_blkno(it also 
needs to be CoW since it is within this refcounted extent), so if we can 
CoW it out now, maybe we have a chance to not call ocfs2_refcount_cow later.

Regards,
Tao