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

[Joel Becker <Joel.Becker@oracle.com>]

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.

> >+	/* 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?

> 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.

Joel

-- 

"Where are my angels?
 Where's my golden one?
 And where is my hope
 Now that my heroes are gone?"

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127