[Ocfs2-tools-devel] [PATCH] Compress dirs option in fsck

[Goldwyn Rodrigues]

Hi,

Thanks for the review.

On Wed, Aug 26, 2009 at 05:57:01PM -0700, Joel Becker wrote:
> On Wed, Aug 19, 2009 at 05:14:59PM -0500, Goldwyn Rodrigues wrote:
<snipped>
> 
> 	You need to use memmove() here.  Even if the memory work didn't
> overlap (and it can), saving a cycle or two isn't going to matter when
> our limiting factor is the dirblock I/O.

Okay. since the write index is always lower, I thought it would be fine.
However, memmove() is safer.

> 
> > +				memcpy(dd->dirblock_buf + write_off,
> > +						dd->dirblock_buf + offset,
> > +						dirent->rec_len);
> > +				dirent = dd->dirblock_buf + write_off;
> 
> 	This needs a cast as you assign it.
> 

Yes, I have incorporated this in the next patch.

> >  	}
> 
> 	What happens if:
> 
> 1) The first dirent is an empty one.
> 2) The second dirent has a problem that is fixed.  DIRENT_CHANGED is set.
>    The second dirent is memmove()d to the front of the block, with
>    dirent->rec_len = REC_LEN(name_len).
> 3) The third dirent has a problem that cannot be fixed, and we jump to
>    out:
> 4) ocfs2_dir_block_iterate() writes the dirblock because of
>    DIRENT_CHANGED.

Oh yes, I missed this case.

> 
> 	What you'll have is a new first dirent with a rec_len that
> doesn't cover the space between it and the next dirent.  The next fsck
> will then be forced to wipe the entire dirblock.
> 	Instead, when you shift a dirent, you need to set its rec_len to
> cover the space between its new location and its old end.    Something
> like:
> 
> +		saved_reclen=dirent->rec_len;
> +		if ((dd->ost->ost_rehash_dirs) && dirent->inode) {
> +			if (write_off < offset) {
> +				/* Bring prev->rec_len back to write_off */
> +				if (prev)
> +					prev->rec_len =
> +						OCFS2_DIR_REC_LEN(prev->name_len);
> +				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 = dd->dirblock_buf + write_off;
> +				/* Cover space from our new location to
> +				 * the next dirent */
> +				dirent->rec_len = offset + saved_reclen
> +					- write_off;
> +			}
> +			write_off += OCFS2_DIR_REC_LEN(dirent->name_len);
> +			ret_flags |= OCFS2_DIRENT_CHANGED;
> +		}
> 

I changed it a bit to compute the write_off in the starting of the block
for the consecutive entry. It is just easier to handle. I will post the
updated patch soon once I have done my share of testing.

Thanks,

-- 
Goldwyn