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 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 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 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 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 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, 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/
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/
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/
Review status (Re: [PATCH] LogFS take three)
Most of my homework is done. There are six items left plus another five I believe should not get changed. Changed: o Kconfig description updated o Spaces converted to tabs in Makefile o Sorted/seperated #includes o structures are __packed instead of packed, #define removed o removed TRACE() o 32bit magic is u instead of ull now o #defines aligned o Removed obsolete #defines, documented rest o LOGFS_SUPER and LOGFS_INODE are inline functions now o removed alloc_disk_sum and free_disk_sum o moved dir_callback up o removed pointless cast from max() parameter o moved LOGFS_LINK_MAX to header o license headers o removed logfs_writepage() o introduced and documented LOGFS_SEGMENT_RESERVE o documented completion dance in mtderase() o removed isbad() check from mtderase() o removed logfs_device_getpage() and logfs_cached_read() o resurrected code to pick correct device o removed beXX typedefs o removed safe_read() o removed wrapper in logfs_cleanup_gc() o device_read() returns error o renamed logfs_alloc_blocks() to match current implementation o kerneldoc comments for structures o removed logfs_compress_none and logfs_uncompress_none o Split header o move fsck behind debug option Changed many times (and I sincerely hope not to have missed any): o removed // comments, usually dead code o added newline between variable declarations and code o moved tail comments o minor code cleanups o changed comment style o added loglevels to printk Unchanged: o enum for OBJ_TOP_JOURNAL and friends Afaics all the potential enums end up being converted through cpu_to_64XX, I'll have to see if that causes more problems than the enums can hope to solve. o generic function for logfs_type() o generic helper for logfs_prepare_write() o removed EOF o error handling o move rootdir generation back to mkfs Won't happen (unless I get convinced to do otherwise): o lowercase LOGFS_SUPER and LOGFS_INODE These simply match the common pattern used is many Linux filesystems. o remove "extern" For structures, this keyword actually serves a purpose and makes sense. On the other hand, it is completely bogus for functions and I won't be bullied into adding bogus crud either. o Move fs/logfs/Locking to Documentation/ At least JFFS2 has such documentation in its own directory as well. I can see good arguments for either side and no strong consensus. While I ultimately just don't case, doing nothing is a good option in cases of great uncertainty. o change (void*) to "proper" cast Neither variant is much better than the other. I'm open for suggestions to completely remove the casts, though. o Change LOGFS_BUG() and LOGFS_BUG_ON() to inline functions These are macros for very much the same reasons BUG() and BUG_ON() are. Jörn -- To my face you have the audacity to advise me to become a thief - the worst kind of thief that is conceivable, a thief of spiritual things, a thief of ideas! It is insufferable, intolerable! -- M. Binet in Scarabouche - 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/
Review status (Re: [PATCH] LogFS take three)
Most of my homework is done. There are six items left plus another five I believe should not get changed. Changed: o Kconfig description updated o Spaces converted to tabs in Makefile o Sorted/seperated #includes o structures are __packed instead of packed, #define removed o removed TRACE() o 32bit magic is u instead of ull now o #defines aligned o Removed obsolete #defines, documented rest o LOGFS_SUPER and LOGFS_INODE are inline functions now o removed alloc_disk_sum and free_disk_sum o moved dir_callback up o removed pointless cast from max() parameter o moved LOGFS_LINK_MAX to header o license headers o removed logfs_writepage() o introduced and documented LOGFS_SEGMENT_RESERVE o documented completion dance in mtderase() o removed isbad() check from mtderase() o removed logfs_device_getpage() and logfs_cached_read() o resurrected code to pick correct device o removed beXX typedefs o removed safe_read() o removed wrapper in logfs_cleanup_gc() o device_read() returns error o renamed logfs_alloc_blocks() to match current implementation o kerneldoc comments for structures o removed logfs_compress_none and logfs_uncompress_none o Split header o move fsck behind debug option Changed many times (and I sincerely hope not to have missed any): o removed // comments, usually dead code o added newline between variable declarations and code o moved tail comments o minor code cleanups o changed comment style o added loglevels to printk Unchanged: o enum for OBJ_TOP_JOURNAL and friends Afaics all the potential enums end up being converted through cpu_to_64XX, I'll have to see if that causes more problems than the enums can hope to solve. o generic function for logfs_type() o generic helper for logfs_prepare_write() o removed EOF o error handling o move rootdir generation back to mkfs Won't happen (unless I get convinced to do otherwise): o lowercase LOGFS_SUPER and LOGFS_INODE These simply match the common pattern used is many Linux filesystems. o remove extern For structures, this keyword actually serves a purpose and makes sense. On the other hand, it is completely bogus for functions and I won't be bullied into adding bogus crud either. o Move fs/logfs/Locking to Documentation/ At least JFFS2 has such documentation in its own directory as well. I can see good arguments for either side and no strong consensus. While I ultimately just don't case, doing nothing is a good option in cases of great uncertainty. o change (void*) to proper cast Neither variant is much better than the other. I'm open for suggestions to completely remove the casts, though. o Change LOGFS_BUG() and LOGFS_BUG_ON() to inline functions These are macros for very much the same reasons BUG() and BUG_ON() are. Jörn -- To my face you have the audacity to advise me to become a thief - the worst kind of thief that is conceivable, a thief of spiritual things, a thief of ideas! It is insufferable, intolerable! -- M. Binet in Scarabouche - 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/
