Re: Review status (Re: [PATCH] LogFS take three)
On Wed, May 23, 2007 at 05:14:04PM +0200, Jörn Engel ([EMAIL PROTECTED]) wrote:
> > > I'm just a German. Forgive me if I drink lesser beverages.
> >
> > You should definitely change that.
>
> Change being German? Not a bad idea, actually.
You cook up really tasty shnaps, in small quantities it is good for
health in infinite volumes.
> > Btw, what about this piece:
> >
> > int logfs_erase_segment(struct super_block *sb, u32 index)
> > {
> > struct logfs_super *super = LOGFS_SUPER(sb);
> >
> > super->s_gec++;
> >
> > return mtderase(sb, index << super->s_segshift, super->s_segsize);
> > }
> >
> > index << super->s_segshift might overflow, mtderase expects loff_t
> > there, since index can be arbitrary segment number, is it possible, that
> > overflow really occurs?
>
> Indeed it is. You just earned your second beer^Wvodka.
Actually this code looks less encrypted than ext2 for, which definitely
a good sign from reviewer's point of view.
> Jörn
--
Evgeniy Polyakov
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Review status (Re: [PATCH] LogFS take three)
On Wed, 23 May 2007 19:07:32 +0400, Evgeniy Polyakov wrote:
> On Wed, May 23, 2007 at 02:58:41PM +0200, Jörn Engel ([EMAIL PROTECTED])
> wrote:
> > On Sun, 20 May 2007 21:30:52 +0400, Evgeniy Polyakov wrote:
>
> And what if it is 33 bits? Or it is not allowed?
Not allowed. Both number and size of segments may never exceed 32bit.
> > > segsize is long, but should be u64 I think.
> >
> > It could be s32 as well.
>
> It is a matter of definition - if segment size is allowed to be more
> than 32 bits, then below transformation is not correct, otherwise
> segment size should not use additional 32bits on 64bit platform, since
> it is long.
I guess I could save 4 Bytes there.
> > I'm just a German. Forgive me if I drink lesser beverages.
>
> You should definitely change that.
Change being German? Not a bad idea, actually.
> Btw, what about this piece:
>
> int logfs_erase_segment(struct super_block *sb, u32 index)
> {
> struct logfs_super *super = LOGFS_SUPER(sb);
>
> super->s_gec++;
>
> return mtderase(sb, index << super->s_segshift, super->s_segsize);
> }
>
> index << super->s_segshift might overflow, mtderase expects loff_t
> there, since index can be arbitrary segment number, is it possible, that
> overflow really occurs?
Indeed it is. You just earned your second beer^Wvodka.
Jörn
--
The wise man seeks everything in himself; the ignorant man tries to get
everything from somebody else.
-- unknown
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Review status (Re: [PATCH] LogFS take three)
On Wed, May 23, 2007 at 02:58:41PM +0200, Jörn Engel ([EMAIL PROTECTED]) wrote:
> On Sun, 20 May 2007 21:30:52 +0400, Evgeniy Polyakov wrote:
> >
> > In that case segment size must be more than 32 bits, or below
> > transformation will not be correct?
>
> Must it? If segment size is just 20bit then the filesystem may only be
> 52bit. Or 51bit when using signed values.
And what if it is 33 bits? Or it is not allowed?
> > segsize is long, but should be u64 I think.
>
> It could be s32 as well.
It is a matter of definition - if segment size is allowed to be more
than 32 bits, then below transformation is not correct, otherwise
segment size should not use additional 32bits on 64bit platform, since
it is long.
> > static void fixup_from_wbuf(struct super_block *sb, struct logfs_area
> > *area, void *read, u64 ofs, size_t readlen)
> >
> > u32 read_start = ofs & (super->s_segsize - 1);
> > u32 read_end = read_start + readlen;
> >
> > And this can overflow, since readlen is size_t.
>
> Theoretically yes. Practically readlen is bounded to sb->blocksize plus
> one header. I'll start worrying about that when blocksize approaches
> 32bit limit.
>
> > > If anyone can find similar bugs, the bounty is a beer or non-alcoholic
> > > beverage of choice. :)
> >
> > Stop kiling your kidneys, your health and promote such antisocial style
> > of life, start drinking vodka instead.
>
> I'm just a German. Forgive me if I drink lesser beverages.
You should definitely change that.
Btw, what about this piece:
int logfs_erase_segment(struct super_block *sb, u32 index)
{
struct logfs_super *super = LOGFS_SUPER(sb);
super->s_gec++;
return mtderase(sb, index << super->s_segshift, super->s_segsize);
}
index << super->s_segshift might overflow, mtderase expects loff_t
there, since index can be arbitrary segment number, is it possible, that
overflow really occurs?
> Jörn
>
> --
> Eighty percent of success is showing up.
> -- Woody Allen
--
Evgeniy Polyakov
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Review status (Re: [PATCH] LogFS take three)
On Sun, 20 May 2007 21:30:52 +0400, Evgeniy Polyakov wrote: > > In that case segment size must be more than 32 bits, or below > transformation will not be correct? Must it? If segment size is just 20bit then the filesystem may only be 52bit. Or 51bit when using signed values. > segsize is long, but should be u64 I think. It could be s32 as well. > static void fixup_from_wbuf(struct super_block *sb, struct logfs_area > *area, void *read, u64 ofs, size_t readlen) > > u32 read_start = ofs & (super->s_segsize - 1); > u32 read_end = read_start + readlen; > > And this can overflow, since readlen is size_t. Theoretically yes. Practically readlen is bounded to sb->blocksize plus one header. I'll start worrying about that when blocksize approaches 32bit limit. > > If anyone can find similar bugs, the bounty is a beer or non-alcoholic > > beverage of choice. :) > > Stop kiling your kidneys, your health and promote such antisocial style > of life, start drinking vodka instead. I'm just a German. Forgive me if I drink lesser beverages. Jörn -- Eighty percent of success is showing up. -- Woody Allen - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Review status (Re: [PATCH] LogFS take three)
On Thu, May 17, 2007 at 07:10:19PM +0200, Jörn Engel ([EMAIL PROTECTED]) wrote: > On Thu, 17 May 2007 20:03:11 +0400, Evgeniy Polyakov wrote: > > > > Is logfs 32bit fs or 674bit, since although you use 64bit values for > > offsets, area management and strange converstions like described below > > from offset into segment number are performed in 32bit? > > Is it enough for SSD for example to be 32bit only? Or if it is 64bit, > > could you please explain logic behind area management? > > Ignoring bugs and signed return values for error handling, it is either > 64bit or 32+32bit. > > Inode numbers and file positions are 64bit. Offsets are 64bit as well. > In a couple of places, offsets are also 32+32bit. Basically the high > bits contain the segment number, the lower bits the offset within a > segment. In that case segment size must be more than 32 bits, or below transformation will not be correct? segsize is long, but should be u64 I think. static void fixup_from_wbuf(struct super_block *sb, struct logfs_area *area, void *read, u64 ofs, size_t readlen) u32 read_start = ofs & (super->s_segsize - 1); u32 read_end = read_start + readlen; And this can overflow, since readlen is size_t. It is wbuf fixup, but I saw that somewhere else. Although, according to your description, it should be 32bit, sum can be more than 32 bit. > If anyone can find similar bugs, the bounty is a beer or non-alcoholic > beverage of choice. :) Stop kiling your kidneys, your health and promote such antisocial style of life, start drinking vodka instead. > Jörn -- Evgeniy Polyakov - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Review status (Re: [PATCH] LogFS take three)
On Thu, 17 May 2007 20:03:11 +0400, Evgeniy Polyakov wrote: > > Is logfs 32bit fs or 674bit, since although you use 64bit values for > offsets, area management and strange converstions like described below > from offset into segment number are performed in 32bit? > Is it enough for SSD for example to be 32bit only? Or if it is 64bit, > could you please explain logic behind area management? Ignoring bugs and signed return values for error handling, it is either 64bit or 32+32bit. Inode numbers and file positions are 64bit. Offsets are 64bit as well. In a couple of places, offsets are also 32+32bit. Basically the high bits contain the segment number, the lower bits the offset within a segment. Side note: It would be nicer if the high 32bit were segment number. Instead the number of bits depends on segment size. Guess I should change that while the format isn't fixed yet. An "area" is a segment that is currently being written. Data is appended to this segment as it comes in, until the segment is full. Any functions dealing with areas only need a 32bit offset, which is the offset within the area, not the absolute device offset. Writes within an area are also buffered. New data first goes into the write buffer (wbuf) and only when this is full is it flushed to the device. NAND flash and some NOR flashes require such buffering. When writing to the device, the 32bit segno and the 32bit in-segment offset need to get converted back to a 64bit device offset. > I've found that you store segment numbers as 32bit values (for example > in prepare_write()), and convert requested 64bit offset into segment > number via superblock's s_segshift. Yes, as described above. > This conversation seems confusing to me in case of real 64bit offsets. > For example this one obtained via prepare_write: > > 7 1 logfs_prepare_write78 fs/logfs/file.c > 8 2 logfs_readpage_nolock20 fs/logfs/file.c > 9 1 logfs_read_block 351 fs/logfs/readwrite.c > 10 1 logfs_read_loop 139 fs/logfs/readwrite.c > 11 2 logfs_segment_read 108 fs/logfs/readwrite.c > 12 1 wbuf_read 289 > > u32 segno = ofs >> super->s_segshift; > > ofs is originally obtained from inode's li_data array, which is filled > with raw segment numbers which can be 64bit (here is another issue, > since logfs_segment_write() returns signed, so essentially logfs is > 63bit filesystem). The filesystem format is 64bit. The current code can only deal with 63bit. Eric Sandeen just fixed ext2 to actually deal with 32bit numbers and the same is possible for logfs. If anyone ever cares... > But here I've came to area management in logfs, and found that it is > 32bit only, for example __logfs_segment_write()/__logfs_get_free_bytes() > returns signed 32 bit value (so it is reduced to 31 bit), which is then > placed into li_data as 64bit value. The latter > (__logfs_get_free_bytes()) truncates 64bit data value obtained via > dev_ofs() into signed 32 bit value. That indeed is a bug. __logfs_get_free_bytes() should return s64 instead of s32. Will fix immediatly. If anyone can find similar bugs, the bounty is a beer or non-alcoholic beverage of choice. :) Jörn -- To announce that there must be no criticism of the President, or that we are to stand by the President, right or wrong, is not only unpatriotic and servile, but is morally treasonable to the American public. -- Theodore Roosevelt, Kansas City Star, 1918 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Review status (Re: [PATCH] LogFS take three)
Hi Jörn. Is logfs 32bit fs or 674bit, since although you use 64bit values for offsets, area management and strange converstions like described below from offset into segment number are performed in 32bit? Is it enough for SSD for example to be 32bit only? Or if it is 64bit, could you please explain logic behind area management? I've found that you store segment numbers as 32bit values (for example in prepare_write()), and convert requested 64bit offset into segment number via superblock's s_segshift. This conversation seems confusing to me in case of real 64bit offsets. For example this one obtained via prepare_write: 7 1 logfs_prepare_write78 fs/logfs/file.c 8 2 logfs_readpage_nolock20 fs/logfs/file.c 9 1 logfs_read_block 351 fs/logfs/readwrite.c 10 1 logfs_read_loop 139 fs/logfs/readwrite.c 11 2 logfs_segment_read 108 fs/logfs/readwrite.c 12 1 wbuf_read 289 u32 segno = ofs >> super->s_segshift; ofs is originally obtained from inode's li_data array, which is filled with raw segment numbers which can be 64bit (here is another issue, since logfs_segment_write() returns signed, so essentially logfs is 63bit filesystem). But here I've came to area management in logfs, and found that it is 32bit only, for example __logfs_segment_write()/__logfs_get_free_bytes() returns signed 32 bit value (so it is reduced to 31 bit), which is then placed into li_data as 64bit value. The latter (__logfs_get_free_bytes()) truncates 64bit data value obtained via dev_ofs() into signed 32 bit value. -- Evgeniy Polyakov - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
