lkml.org 
[lkml]   [2010]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] ocfs2: Zero the tail cluster when extending past i_size v2
    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


    \
     
     \ /
      Last update: 2010-07-06 09:23    [W:0.029 / U:148.780 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site