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: [email protected]
Phone: (650) 506-8127
_______________________________________________
Ocfs2-devel mailing list
[email protected]
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
