| Message ID | 1305036366-1228-1-git-send-email-xiaoqiangnk@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, 10 May 2011, Yongqiang Yang wrote: > v1->v2: > Add more specific description. > > To get delayed-extent information, ext4_ext_fiemap_cb() lookups pagecache, > it thus collects information starting from a page's head block. > If blockszie < pagesize, the beginning blocks of a page may lie before the > range. ext4_ext_fiemap_cb() should proceed ignoring them, because they > has been handled before. Thanks for the description, but I have one question below. > > Reported-by: Amir Goldstein <amir73il@users.sf.net> > Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com> > --- > fs/ext4/extents.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index e363f21..ec37109 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -3718,9 +3718,14 @@ out: > > bh = head; > do { > + if (end < newex->ec_block) > + /* The buffer is not in > + * the request range. > + */ > + continue; So if the condition is true, then we might leave the loop, because (bh == head) in the first iteration, is that desired behavior? Also if we hit this condition in other than first iteration we will spin forever. I do not think this is right. How did you tested this case ? Thanks! -Lukas > if (buffer_mapped(bh)) { > /* get the 1st mapped buffer. */ > - if (end > newex->ec_block + > + if (end >= newex->ec_block + > newex->ec_len) > /* The buffer is out of > * the request range. > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 10, 2011 at 10:35 PM, Lukas Czerner <lczerner@redhat.com> wrote: > On Tue, 10 May 2011, Yongqiang Yang wrote: > >> v1->v2: >> Add more specific description. >> >> To get delayed-extent information, ext4_ext_fiemap_cb() lookups pagecache, >> it thus collects information starting from a page's head block. >> If blockszie < pagesize, the beginning blocks of a page may lie before the >> range. ext4_ext_fiemap_cb() should proceed ignoring them, because they >> has been handled before. > > Thanks for the description, but I have one question below. > >> >> Reported-by: Amir Goldstein <amir73il@users.sf.net> >> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com> >> --- >> fs/ext4/extents.c | 7 ++++++- >> 1 files changed, 6 insertions(+), 1 deletions(-) >> >> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >> index e363f21..ec37109 100644 >> --- a/fs/ext4/extents.c >> +++ b/fs/ext4/extents.c >> @@ -3718,9 +3718,14 @@ out: >> >> bh = head; >> do { >> + if (end < newex->ec_block) >> + /* The buffer is not in >> + * the request range. >> + */ >> + continue; > So if the condition is true, then we might leave the loop, because (bh == > head) in the first iteration, is that desired behavior? Also if we hit > this condition in other than first iteration we will spin forever. I do > not think this is right. How did you tested this case ? You are right. Maybe the patch sent out is not same as the patch with which I tested. What a stupid error! I am not sure what happened now. I will check the patch with which I tested tomorrow. My working tree passed xfstests 225 in both 1k blocksize and 4k blocksize cases. Sorry! Yongqiang. > > Thanks! > -Lukas > >> if (buffer_mapped(bh)) { >> /* get the 1st mapped buffer. */ >> - if (end > newex->ec_block + >> + if (end >= newex->ec_block + >> newex->ec_len) >> /* The buffer is out of >> * the request range. >> >
On Tue, May 10, 2011 at 11:08 PM, Yongqiang Yang <xiaoqiangnk@gmail.com> wrote: > On Tue, May 10, 2011 at 10:35 PM, Lukas Czerner <lczerner@redhat.com> wrote: >> On Tue, 10 May 2011, Yongqiang Yang wrote: >> >>> v1->v2: >>> Add more specific description. >>> >>> To get delayed-extent information, ext4_ext_fiemap_cb() lookups pagecache, >>> it thus collects information starting from a page's head block. >>> If blockszie < pagesize, the beginning blocks of a page may lie before the >>> range. ext4_ext_fiemap_cb() should proceed ignoring them, because they >>> has been handled before. >> >> Thanks for the description, but I have one question below. >> >>> >>> Reported-by: Amir Goldstein <amir73il@users.sf.net> >>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com> >>> --- >>> fs/ext4/extents.c | 7 ++++++- >>> 1 files changed, 6 insertions(+), 1 deletions(-) >>> >>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >>> index e363f21..ec37109 100644 >>> --- a/fs/ext4/extents.c >>> +++ b/fs/ext4/extents.c >>> @@ -3718,9 +3718,14 @@ out: >>> >>> bh = head; >>> do { >>> + if (end < newex->ec_block) >>> + /* The buffer is not in >>> + * the request range. >>> + */ >>> + continue; >> So if the condition is true, then we might leave the loop, because (bh == >> head) in the first iteration, is that desired behavior? Also if we hit >> this condition in other than first iteration we will spin forever. I do >> not think this is right. How did you tested this case ? > You are right. Maybe the patch sent out is not same as the patch with > which I tested. What a stupid error! > I am not sure what happened now. I will check the patch with which I > tested tomorrow. > > My working tree passed xfstests 225 in both 1k blocksize and 4k > blocksize cases. I checked the code I tested with. Indeed I tested with this ugly patch and xfstests 225 passed. I tested again and results were same ' PASSED'. I had a look into xfstests 225 and found that there are problems in xfstests 225 so it can not find the bug induced by this ugly patch. It seems that current xfstests 225 has a lot of problems. I am fixing xfstests 225 to enable it find this bug. Yongqiang. > > Sorry! > > Yongqiang. >> >> Thanks! >> -Lukas >> >>> if (buffer_mapped(bh)) { >>> /* get the 1st mapped buffer. */ >>> - if (end > newex->ec_block + >>> + if (end >= newex->ec_block + >>> newex->ec_len) >>> /* The buffer is out of >>> * the request range. >>> >> > > > > -- > Best Wishes > Yongqiang Yang >
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index e363f21..ec37109 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -3718,9 +3718,14 @@ out: bh = head; do { + if (end < newex->ec_block) + /* The buffer is not in + * the request range. + */ + continue; if (buffer_mapped(bh)) { /* get the 1st mapped buffer. */ - if (end > newex->ec_block + + if (end >= newex->ec_block + newex->ec_len) /* The buffer is out of * the request range.
v1->v2: Add more specific description. To get delayed-extent information, ext4_ext_fiemap_cb() lookups pagecache, it thus collects information starting from a page's head block. If blockszie < pagesize, the beginning blocks of a page may lie before the range. ext4_ext_fiemap_cb() should proceed ignoring them, because they has been handled before. Reported-by: Amir Goldstein <amir73il@users.sf.net> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com> --- fs/ext4/extents.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)