[Ocfs2-devel] [PATCH 1/1] Ocfs2: Optimize truncting codes for ocfs2 to use ocfs2_remove_btree_range instead.

[tristan]

Tao Ma wrote:
> Hi Tristan,
>     Sorry for the delay. This patch looks good. Just some minor comments.
> Tristan Ye wrote:
> <snip>
>>  int ocfs2_commit_truncate(struct ocfs2_super *osb,
>>                struct inode *inode,
>> -              struct buffer_head *fe_bh,
>> -              struct ocfs2_truncate_context *tc)
>> +              struct buffer_head *fe_bh)
>>  {
>> -    int status, i, credits, tl_sem = 0;
>> -    u32 clusters_to_del, new_highest_cpos, range;
>> +    int status = 0, i, flags = 0;
>> +    u32 new_highest_cpos, range, trunc_cpos, trunc_len, phys_cpos, 
>> coff;
>>      u64 blkno = 0;
>> +
> Why you always want to separate the declaration with blank lines? I 
> have never seen such one in the original codes. So please remove this 
> redundant line. The same goes for the following codes.


Sorry, maybe I originally wanted to separate structure definitions from 
simple primitives, I can remove this anyway:)


>>      struct ocfs2_extent_list *el;
>> -    handle_t *handle = NULL;
>> -    struct inode *tl_inode = osb->osb_tl_inode;
>> +    struct ocfs2_extent_rec *rec;
>>      struct ocfs2_path *path = NULL;
>>      struct ocfs2_dinode *di = (struct ocfs2_dinode *)fe_bh->b_data;
>> -    struct ocfs2_alloc_context *meta_ac = NULL;
>> -    struct ocfs2_refcount_tree *ref_tree = NULL;
>> +    u64 refcount_loc = le64_to_cpu(di->i_refcount_loc);
>> +
>> +    struct ocfs2_extent_tree et;
>> +    struct ocfs2_cached_dealloc_ctxt dealloc;
>>  
>>      mlog_entry_void();
>> +   
>> +    ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), fe_bh);
>> +    ocfs2_init_dealloc_ctxt(&dealloc);
>>  
>>      new_highest_cpos = ocfs2_clusters_for_bytes(osb->sb,
>>                               i_size_read(inode));
>> @@ -7433,8 +7110,6 @@ int ocfs2_commit_truncate(struct ocfs2_super *osb,
>>          goto bail;
>>      }
>>  
>> -    ocfs2_extent_map_trunc(inode, new_highest_cpos);
>> -
> I remembered that I have said this line should be reserved.
> Oh, I just checked the previous e-mail and you replied "I guess it's 
> safe to remove extent map here, since ocfs2_remove_extent() will do 
> the same. ". This isn't right.
> Check the ocfs2_remove_extent, and the code looks like this:
>     /*
>       * XXX: Why are we truncating to 0 instead of wherever this
>      * affects us?
>      */
>     ocfs2_et_extent_map_truncate(et, 0);
> So you see, it is "XXX", and we may change it somehow later. We can't 
> trust it.
>

That's OK.


>>  start:
>>      /*
>>       * Check that we still have allocation to delete.
>> @@ -7444,8 +7119,6 @@ start:
>>          goto bail;
>>      }
>>  
>> -    credits = 0;
>> -
>>      /*
>>       * Truncate always works against the rightmost tree branch.
>>       */
>> @@ -7480,101 +7153,56 @@ start:
>>      }
>>  
>>      i = le16_to_cpu(el->l_next_free_rec) - 1;
>> -    range = le32_to_cpu(el->l_recs[i].e_cpos) +
>> -        ocfs2_rec_clusters(el, &el->l_recs[i]);
>> -    if (i == 0 && ocfs2_is_empty_extent(&el->l_recs[i])) {
>> -        clusters_to_del = 0;
>> -    } else if (le32_to_cpu(el->l_recs[i].e_cpos) >= new_highest_cpos) {
>> -        clusters_to_del = ocfs2_rec_clusters(el, &el->l_recs[i]);
>> -        blkno = le64_to_cpu(el->l_recs[i].e_blkno);
>> +    rec = &el->l_recs[i];
>> +    flags = rec->e_flags;
>> +    range = le32_to_cpu(rec->e_cpos) + ocfs2_rec_clusters(el, rec);
>> +
>> +    if (i == 0 && ocfs2_is_empty_extent(rec)) {
>> +        /*
>> +         * Should remove this extent block.
>> +         */
>> +        trunc_cpos = le32_to_cpu(rec->e_cpos);
>> +        trunc_len = 0;
>> +        coff = 0;
>> +        blkno = 0;
> In this case, I guess we need to work like the original 
> ocfs2_do_truncate:
>     /*
>      * Lower levels depend on this never happening, but it's best
>      * to check it up here before changing the tree.
>      */
>     if (el->l_tree_depth && el->l_recs[0].e_int_clusters == 0) {
>         ocfs2_error(inode->i_sb,
>                 "Inode %lu has an empty extent record, depth %u\n",
>                 inode->i_ino, le16_to_cpu(el->l_tree_depth));
>         status = -EROFS;
>         goto bail;
>     }


Good catch!



>> +    } else if (le32_to_cpu(rec->e_cpos) >= new_highest_cpos) {
>> +        /* +         * Truncate entire record.
>> +         */
>> +        trunc_cpos = le32_to_cpu(rec->e_cpos);
>> +        trunc_len = ocfs2_rec_clusters(el, rec);
>> +        coff = 0;
> I just notice you only use coff in the below "else if"? Please remove it.

Yes, you're right.

>> +        blkno = le64_to_cpu(rec->e_blkno);
>>      } else if (range > new_highest_cpos) {
>> -        clusters_to_del = (ocfs2_rec_clusters(el, &el->l_recs[i]) +
>> -                   le32_to_cpu(el->l_recs[i].e_cpos)) -
>> -                  new_highest_cpos;
>> -        blkno = le64_to_cpu(el->l_recs[i].e_blkno) +
>> -            ocfs2_clusters_to_blocks(inode->i_sb,
>> -                ocfs2_rec_clusters(el, &el->l_recs[i]) -
>> -                clusters_to_del);
>> +        /*
>> +         * Partial truncate. it also should be
>> +         * the last truncate we're doing.
>> +         */
>> +        trunc_cpos = new_highest_cpos;
>> +        trunc_len = range - new_highest_cpos;
>> +        coff = new_highest_cpos - le32_to_cpu(rec->e_cpos);
>> +        blkno = le64_to_cpu(rec->e_blkno) +
>> +                ocfs2_clusters_to_blocks(inode->i_sb, coff);
>>      } else {
>> +        /*
>> +         * Truncate completed, leave happily.
>> +         */
>>          status = 0;
>>          goto bail;
>>      }
>
> Regards,
> Tao