Hello,
sorry for replying to myself but I've just found out that the patch I've
sent was and old version of the patch which had some problems. Attached is
a new version.
On Tue 06-11-07 12:31:42, Jan Kara wrote:
> it seems attached patch still did not get your attention. It makes
> e2fsprogs properly handle filesystems with 64KB block size. Could you put
> it into e2fsprogs git? Thanks.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, Nov 07, 2007 at 05:09:39PM +0100, Jan Kara wrote:
> Subject: Support for 64KB blocksize in ext2-4 directories.
>
> When block size is 64KB, we have to take care that rec_len does not overflow.
> Kernel stores 0xffff in case 0x10000 should be stored - perform appropriate
> conversion when reading from / writing to disk.
NACK. You can't do the conversion in the reader/writer routines
because the fundamentally rec_len is only a 16 bit field. So when you
read a directory block where the rec_len field is encoded as 0xFFFF,
and you translate it to 0x10000, when you assign it to
dirent->rec_len, the 0x10000 gets chopped off and rec_len gets a value
of zero. Did you test this patch before submitting it?
The only way to do this is to find all of the places that reference
rec_len, and do the check there.
- Ted
On Sat 10-11-07 19:37:03, Theodore Tso wrote:
> On Wed, Nov 07, 2007 at 05:09:39PM +0100, Jan Kara wrote:
>
> > Subject: Support for 64KB blocksize in ext2-4 directories.
> >
> > When block size is 64KB, we have to take care that rec_len does not overflow.
> > Kernel stores 0xffff in case 0x10000 should be stored - perform appropriate
> > conversion when reading from / writing to disk.
>
> NACK. You can't do the conversion in the reader/writer routines
> because the fundamentally rec_len is only a 16 bit field. So when you
> read a directory block where the rec_len field is encoded as 0xFFFF,
> and you translate it to 0x10000, when you assign it to
> dirent->rec_len, the 0x10000 gets chopped off and rec_len gets a value
> of zero. Did you test this patch before submitting it?
Argh, stupid me. I've just tested that I didn't break anything for normal
block size and thought that I cannot make mistake in such a simple thing
;).
> The only way to do this is to find all of the places that reference
> rec_len, and do the check there.
Yes.. Thanks for having look.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon, Nov 12, 2007 at 10:52:45AM +0100, Jan Kara wrote:
> > Did you test this patch before submitting it?
>
> Argh, stupid me. I've just tested that I didn't break anything for normal
> block size and thought that I cannot make mistake in such a simple thing
> ;).
Could I ask you to perhaps include some 64k blocksize test cases that
would exercise the new codepaths?
> > The only way to do this is to find all of the places that reference
> > rec_len, and do the check there.
> Yes.. Thanks for having look.
One suggestion is that instead of just creating an conversion
function, and then doing a global search and replace, in some places
it might be better to declare an integer variable, and then assign
"rec_len = ext2fs_rec_len_from_disk(dirent->rec_len)". For example,
that would make ext2fs_process_dir_block() more readable, where
dirent->rec_len is used no less than eight times.
Thanks, and my apologies for not having time to review the patch until
now. At the moment things are a bit crazy since I am effectively
doing two jobs, since I am in transition between two assignments, and
me doing most of both of them at the moment. I should have
substantially more time after the new year begins.
- Ted
On Mon 12-11-07 09:58:23, Theodore Tso wrote:
> On Mon, Nov 12, 2007 at 10:52:45AM +0100, Jan Kara wrote:
> > > Did you test this patch before submitting it?
> >
> > Argh, stupid me. I've just tested that I didn't break anything for normal
> > block size and thought that I cannot make mistake in such a simple thing
> > ;).
>
> Could I ask you to perhaps include some 64k blocksize test cases that
> would exercise the new codepaths?
Fair enough, will do.
> > > The only way to do this is to find all of the places that reference
> > > rec_len, and do the check there.
> > Yes.. Thanks for having look.
>
> One suggestion is that instead of just creating an conversion
> function, and then doing a global search and replace, in some places
> it might be better to declare an integer variable, and then assign
> "rec_len = ext2fs_rec_len_from_disk(dirent->rec_len)". For example,
> that would make ext2fs_process_dir_block() more readable, where
> dirent->rec_len is used no less than eight times.
OK, thanks for suggestion.
> Thanks, and my apologies for not having time to review the patch until
> now. At the moment things are a bit crazy since I am effectively
> doing two jobs, since I am in transition between two assignments, and
> me doing most of both of them at the moment. I should have
> substantially more time after the new year begins.
I see. I'll be patient then :)
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Nov 12, 2007 09:58 -0500, Theodore Tso wrote:
> On Mon, Nov 12, 2007 at 10:52:45AM +0100, Jan Kara wrote:
> > > Did you test this patch before submitting it?
> >
> > Argh, stupid me. I've just tested that I didn't break anything for normal
> > block size and thought that I cannot make mistake in such a simple thing
> > ;).
>
> Could I ask you to perhaps include some 64k blocksize test cases that
> would exercise the new codepaths?
One thing I had done to verify that 64kB rec_len was broken was to always
create lost+found with at least 2 disk blocks, since the second block in
lost+found will not have any dirents in it and will tickle this bug.
Patch attached.
Cheers, Andreas
--
Andreas Dilger
Sr. Software Engineer, Lustre Group
Sun Microsystems of Canada, Inc.