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

[Joel Becker]

On Wed, Aug 19, 2009 at 05:14:59PM -0500, Goldwyn Rodrigues wrote:
> This patch adds the option to compress directories (-D) in fsck.
> The option compresses the directory entries and consolidates a hole at
> the end of the directory block. This consolidated hole increases the
> chance of ocfs2_prepare_dir_for_insert (kmp) to find a hole using
> ocfs2_find_dir_space_* functions while inserting a new directory entry
> in the directory.

	I like the idea.  A couple of comments.

> diff --git a/fsck.ocfs2/pass2.c b/fsck.ocfs2/pass2.c
> index 810bf10..2d9d074 100644
> --- a/fsck.ocfs2/pass2.c
> +++ b/fsck.ocfs2/pass2.c
> @@ -668,6 +668,7 @@ static unsigned
> pass2_dir_block_iterate(o2fsck_dirblock_entry *dbe,
>  	struct dirblock_data *dd = priv_data;
>  	struct ocfs2_dir_entry *dirent, *prev = NULL;
>  	unsigned int offset = 0, ret_flags = 0, end = dd->fs->fs_blocksize;
> +	unsigned int write_off, saved_reclen;
>  	struct ocfs2_dinode *di = (struct ocfs2_dinode *)dd->inoblock_buf;
>  	errcode_t ret = 0;
> 
> @@ -723,6 +724,8 @@ static unsigned
> pass2_dir_block_iterate(o2fsck_dirblock_entry *dbe,
>  			end = ocfs2_dir_trailer_blk_off(dd->fs);
>  	}
> 
> +	write_off = offset;
> +
>  	while (offset < end) {
>  		dirent = (struct ocfs2_dir_entry *)(dd->dirblock_buf + offset);
> 
> @@ -804,8 +807,31 @@ 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_rehash_dirs) && dirent->inode) {
> +			dirent->rec_len = OCFS2_DIR_REC_LEN(dirent->name_len);
> +			if (write_off < offset) {
> +				/* memcpy works good even though
> +				it is the same buffer*/

	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.

> +				verbosef("ino: %llu woff: %u off: %u\n",
> +					dirent->inode, write_off, offset);
> +				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.

> +			}
> +			write_off += dirent->rec_len;
> +			ret_flags |= OCFS2_DIRENT_CHANGED;
> +		}
>  		prev = dirent;
> +		offset += saved_reclen;
> +	}
> +
> +	/* Zero the rest of the block */
> +	if (dd->ost->ost_rehash_dirs) {
> +		memset(dd->dirblock_buf + write_off, '\0', end - write_off);
> +		ret_flags |= OCFS2_DIRENT_CHANGED;
> +		dirent->rec_len = dirent->rec_len + end - write_off;
>  	}

	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.

	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;
+		}

Joel

-- 

"I inject pure kryptonite into my brain.
 It improves my kung fu, and it eases the pain."


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