[Ocfs2-tools-devel] Add compress dirs option in fsck.ocfs2

[Goldwyn Rodrigues]

Hi Joel,

On Wed, Sep 2, 2009 at 5:03 PM, Joel Becker<Joel.Becker at oracle.com> wrote:
> On Tue, Sep 01, 2009 at 06:08:39PM -0500, Goldwyn Rodrigues wrote:
>> On Tue, Sep 1, 2009 at 3:29 PM, Joel Becker<Joel.Becker at oracle.com> wrote:
>> > On Mon, Aug 31, 2009 at 06:47:00PM -0500, Goldwyn Rodrigues wrote:
>> >> @@ -804,8 +807,29 @@ static unsigned pass2_dir_block_iterate(o2fsck_dirblock_entry *dbe,
>> >>                               dirent->name, (uint64_t)dirent->inode);
>> >>               o2fsck_icount_delta(dd->ost->ost_icount_refs, dirent->inode, 1);
>> >>  next:
>> >> -             offset += dirent->rec_len;
>> >> +             saved_reclen = dirent->rec_len;
>> >> +             if (dd->ost->ost_compress_dirs) {
>> >> +                     if (prev && prev->inode) {
>> >> +                             /*Bring previous rec_len to required space */
>> >> +                             prev->rec_len = OCFS2_DIR_REC_LEN(prev->name_len);
>> >> +                             write_off += prev->rec_len;
>> >> +                     }
>> >> +                     if (write_off < offset) {
>> >> +                             verbosef("ino: %llu woff: %u off: %u\n",
>> >> +                                     dirent->inode, write_off, offset);
>> >> +                             memmove(dd->dirblock_buf + write_off,
>> >> +                                     dd->dirblock_buf + offset,
>> >> +                                     OCFS2_DIR_REC_LEN(dirent->name_len));
>> >> +                             dirent = (struct ocfs2_dir_entry *)(dd->dirblock_buf + write_off);
>> >> +                             /* Cover space from our new location to
>> >> +                             * the next dirent */
>> >> +                             dirent->rec_len = saved_reclen + offset - write_off;
>> >> +
>> >> +                             ret_flags |= OCFS2_DIRENT_CHANGED;
>> >> +                     }
>> >> +             }
>> >>               prev = dirent;
>> >> +             offset += saved_reclen;
>> >>       }
>> >
>> >        Two things.  First, what about empty records?  Imagine you've
>> > just seen a dirent, set its rec_len to saved_reclen+offset-write_off.
>> > You set prev=dirent.  Then next pass is a deleted dirent, dirent->inode
>> > = 0.  You skip this block and set prev=dirent.  Then you come to another
>> > valid dirent.  However, prev->inode is zero, and so you won't update the
>> > first dirent's rec_len, nor will you update write_off correctly.
>>
>> No, I do not skip the dirent record if dirent->inode==0. I still
>> memmove() it and set prev to dirent (the empty one).
>>
>> prev is responsible for skipping empty records (by not updating write_off)
>
>        My point is that with prev pointing to the empty one, there's no
> pointer to the last good one for you to update it.

But why do you need the last non-empty prev pointer? The current code
(before this patch) doesn't seem to need it. The only function which
takes the previous pointer is fix_dirent_lengths, and it does not use
it either. I am basing this on the current code structure. If there is
any design principle I have missed, could you point it out to me.


>
>> >        Second thing, when you reach the end of the block there is no
>> > guarantee the last dirent->rec_len covers the entire block.  It will if
>> > it was the last dirent, but not if there were empty ones after it.
>>
>> I was not aware of this. I thought that there are no empty records
>> trailing the last dir_entry, and the last dir_entry always covered the
>> rest of the block. What condition would create this situation?
>
>        I think I wasn't clear.  The directory block you find will
> absolutely have the last dirent covering the remainder of the block.
> Your code doesn't appear to ensure it stays that way as it shifts
> things.

It does. With dirent pointing to the last directory entry,
offset+saved_reclen will evaluate to end(offset).
So, dirent->rec_len = saved_reclen + offset - write_off; evaluates to
cover the rest of the block.

Or perhaps I do not understand the point you are trying to make. Could
you give me a sample case, so that I can do a dry run through the
code?

Regards,

-- 
Goldwyn