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

[Joel Becker]

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.

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

Joel

-- 

"Sometimes I think the surest sign intelligent
 life exists elsewhere in the universe is that
 none of it has tried to contact us."
                                -Calvin & Hobbes

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