| Message ID | 1270113678-5901-1-git-send-email-surbhi.palande@canonical.com |
|---|---|
| State | Accepted |
| Delegated to: | Andy Whitcroft |
| Headers | show |
Ok, looks good to me now. Surbhi Palande wrote: > The following cherry-picked patch fixes the calculation of extents in fiemap. > Without this patch a user of fiemap could be misled into thinking that there > are no extents in a file when actually there are. This patch is necessary for > code such as ureadahead which uses fiemap to find out if the file extents have > increased. > > The commit id in the upstream code is: aca92ff6f57c000d1b4523e383c8bd6b8269b8b1 > This patch fixes bug 474597 on LP. Do consider merging this in Lucid. > > From dfaa33f0502a8bf9dff0e596783a40ce330c7a9b Mon Sep 17 00:00:00 2001 > From: Leonard Michlmayr <leonard.michlmayr@gmail.com> > Date: Thu, 4 Mar 2010 17:07:28 -0500 > Subject: [PATCH] ext4: correctly calculate number of blocks for fiemap > > ext4_fiemap() rounds the length of the requested range down to > blocksize, which is is not the true number of blocks that cover the > requested region. This problem is especially impressive if the user > requests only the first byte of a file: not a single extent will be > reported. > > We fix this by calculating the last block of the region and then > subtract to find the number of blocks in the extents. > > Signed-off-by: Leonard Michlmayr <leonard.michlmayr@gmail.com> > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > (cherry picked from commit aca92ff6f57c000d1b4523e383c8bd6b8269b8b1 upstream) > > BugLink: http://launchpad.net/bugs/474597 > Signed-off-by: Surbhi Palande <surbhi.palande@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > fs/ext4/extents.c | 9 +++++++-- > 1 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index 8b8bae4..f199d20 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -3711,7 +3711,6 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > __u64 start, __u64 len) > { > ext4_lblk_t start_blk; > - ext4_lblk_t len_blks; > int error = 0; > > /* fallback to generic here if not in extents fmt */ > @@ -3725,8 +3724,14 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) { > error = ext4_xattr_fiemap(inode, fieinfo); > } else { > + ext4_lblk_t len_blks; > + __u64 last_blk; > + > start_blk = start >> inode->i_sb->s_blocksize_bits; > - len_blks = len >> inode->i_sb->s_blocksize_bits; > + last_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits; > + if (last_blk >= EXT_MAX_BLOCK) > + last_blk = EXT_MAX_BLOCK-1; > + len_blks = ((ext4_lblk_t) last_blk) - start_blk + 1; > > /* > * Walk the extent tree gathering extent information.
On 04/01/2010 02:37 AM, Stefan Bader wrote: > Ok, looks good to me now. > > Surbhi Palande wrote: >> The following cherry-picked patch fixes the calculation of extents in fiemap. >> Without this patch a user of fiemap could be misled into thinking that there >> are no extents in a file when actually there are. This patch is necessary for >> code such as ureadahead which uses fiemap to find out if the file extents have >> increased. >> >> The commit id in the upstream code is: aca92ff6f57c000d1b4523e383c8bd6b8269b8b1 >> This patch fixes bug 474597 on LP. Do consider merging this in Lucid. >> >> From dfaa33f0502a8bf9dff0e596783a40ce330c7a9b Mon Sep 17 00:00:00 2001 >> From: Leonard Michlmayr<leonard.michlmayr@gmail.com> >> Date: Thu, 4 Mar 2010 17:07:28 -0500 >> Subject: [PATCH] ext4: correctly calculate number of blocks for fiemap >> >> ext4_fiemap() rounds the length of the requested range down to >> blocksize, which is is not the true number of blocks that cover the >> requested region. This problem is especially impressive if the user >> requests only the first byte of a file: not a single extent will be >> reported. >> >> We fix this by calculating the last block of the region and then >> subtract to find the number of blocks in the extents. >> >> Signed-off-by: Leonard Michlmayr<leonard.michlmayr@gmail.com> >> Signed-off-by: "Theodore Ts'o"<tytso@mit.edu> >> (cherry picked from commit aca92ff6f57c000d1b4523e383c8bd6b8269b8b1 upstream) >> >> BugLink: http://launchpad.net/bugs/474597 >> Signed-off-by: Surbhi Palande<surbhi.palande@canonical.com> > Acked-by: Stefan Bader<stefan.bader@canonical.com> >> --- >> fs/ext4/extents.c | 9 +++++++-- >> 1 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >> index 8b8bae4..f199d20 100644 >> --- a/fs/ext4/extents.c >> +++ b/fs/ext4/extents.c >> @@ -3711,7 +3711,6 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, >> __u64 start, __u64 len) >> { >> ext4_lblk_t start_blk; >> - ext4_lblk_t len_blks; >> int error = 0; >> >> /* fallback to generic here if not in extents fmt */ >> @@ -3725,8 +3724,14 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, >> if (fieinfo->fi_flags& FIEMAP_FLAG_XATTR) { >> error = ext4_xattr_fiemap(inode, fieinfo); >> } else { >> + ext4_lblk_t len_blks; >> + __u64 last_blk; >> + >> start_blk = start>> inode->i_sb->s_blocksize_bits; >> - len_blks = len>> inode->i_sb->s_blocksize_bits; >> + last_blk = (start + len - 1)>> inode->i_sb->s_blocksize_bits; >> + if (last_blk>= EXT_MAX_BLOCK) >> + last_blk = EXT_MAX_BLOCK-1; >> + len_blks = ((ext4_lblk_t) last_blk) - start_blk + 1; >> >> /* >> * Walk the extent tree gathering extent information. > > Maybe I'm misreading this bug according to https://bugs.launchpad.net/ubuntu/+source/linux/+bug/474597/comments/7 ted applied a different patch to fix this issue. Brad
On Thu, 2010-04-01 at 07:53 -0700, Brad Figg wrote: > On 04/01/2010 02:37 AM, Stefan Bader wrote: > > Ok, looks good to me now. > > > > Surbhi Palande wrote: > >> The following cherry-picked patch fixes the calculation of extents in fiemap. > >> Without this patch a user of fiemap could be misled into thinking that there > >> are no extents in a file when actually there are. This patch is necessary for > >> code such as ureadahead which uses fiemap to find out if the file extents have > >> increased. > >> > >> The commit id in the upstream code is: aca92ff6f57c000d1b4523e383c8bd6b8269b8b1 > >> This patch fixes bug 474597 on LP. Do consider merging this in Lucid. > >> > >> From dfaa33f0502a8bf9dff0e596783a40ce330c7a9b Mon Sep 17 00:00:00 2001 > >> From: Leonard Michlmayr<leonard.michlmayr@gmail.com> > >> Date: Thu, 4 Mar 2010 17:07:28 -0500 > >> Subject: [PATCH] ext4: correctly calculate number of blocks for fiemap > >> > >> ext4_fiemap() rounds the length of the requested range down to > >> blocksize, which is is not the true number of blocks that cover the > >> requested region. This problem is especially impressive if the user > >> requests only the first byte of a file: not a single extent will be > >> reported. > >> > >> We fix this by calculating the last block of the region and then > >> subtract to find the number of blocks in the extents. > >> > >> Signed-off-by: Leonard Michlmayr<leonard.michlmayr@gmail.com> > >> Signed-off-by: "Theodore Ts'o"<tytso@mit.edu> > >> (cherry picked from commit aca92ff6f57c000d1b4523e383c8bd6b8269b8b1 upstream) > >> > >> BugLink: http://launchpad.net/bugs/474597 > >> Signed-off-by: Surbhi Palande<surbhi.palande@canonical.com> > > Acked-by: Stefan Bader<stefan.bader@canonical.com> > >> --- > >> fs/ext4/extents.c | 9 +++++++-- > >> 1 files changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > >> index 8b8bae4..f199d20 100644 > >> --- a/fs/ext4/extents.c > >> +++ b/fs/ext4/extents.c > >> @@ -3711,7 +3711,6 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > >> __u64 start, __u64 len) > >> { > >> ext4_lblk_t start_blk; > >> - ext4_lblk_t len_blks; > >> int error = 0; > >> > >> /* fallback to generic here if not in extents fmt */ > >> @@ -3725,8 +3724,14 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > >> if (fieinfo->fi_flags& FIEMAP_FLAG_XATTR) { > >> error = ext4_xattr_fiemap(inode, fieinfo); > >> } else { > >> + ext4_lblk_t len_blks; > >> + __u64 last_blk; > >> + > >> start_blk = start>> inode->i_sb->s_blocksize_bits; > >> - len_blks = len>> inode->i_sb->s_blocksize_bits; > >> + last_blk = (start + len - 1)>> inode->i_sb->s_blocksize_bits; > >> + if (last_blk>= EXT_MAX_BLOCK) > >> + last_blk = EXT_MAX_BLOCK-1; > >> + len_blks = ((ext4_lblk_t) last_blk) - start_blk + 1; > >> > >> /* > >> * Walk the extent tree gathering extent information. > > > > > > Maybe I'm misreading this bug according to > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/474597/comments/7 > ted applied a different patch to fix this issue. > Brad, thanks for pointing this out. The patch that actually got into the linux kernel does seem to have the additional check (missing from whats on launchpad) +if (last_blk>= EXT_MAX_BLOCK) + last_blk = EXT_MAX_BLOCK-1; This check seems necessary for the corner case. I have pointed the difference out on the buglink too. Warm Regards, Surbhi. > Brad
On Fri, Apr 02, 2010 at 01:44:41AM +0300, Surbhi Palande wrote: > On Thu, 2010-04-01 at 07:53 -0700, Brad Figg wrote: > > On 04/01/2010 02:37 AM, Stefan Bader wrote: > > > Ok, looks good to me now. > > > > > > Surbhi Palande wrote: > > >> The following cherry-picked patch fixes the calculation of extents in fiemap. > > >> Without this patch a user of fiemap could be misled into thinking that there > > >> are no extents in a file when actually there are. This patch is necessary for > > >> code such as ureadahead which uses fiemap to find out if the file extents have > > >> increased. > > >> > > >> The commit id in the upstream code is: aca92ff6f57c000d1b4523e383c8bd6b8269b8b1 > > >> This patch fixes bug 474597 on LP. Do consider merging this in Lucid. > > >> > > >> From dfaa33f0502a8bf9dff0e596783a40ce330c7a9b Mon Sep 17 00:00:00 2001 > > >> From: Leonard Michlmayr<leonard.michlmayr@gmail.com> > > >> Date: Thu, 4 Mar 2010 17:07:28 -0500 > > >> Subject: [PATCH] ext4: correctly calculate number of blocks for fiemap > > >> > > >> ext4_fiemap() rounds the length of the requested range down to > > >> blocksize, which is is not the true number of blocks that cover the > > >> requested region. This problem is especially impressive if the user > > >> requests only the first byte of a file: not a single extent will be > > >> reported. > > >> > > >> We fix this by calculating the last block of the region and then > > >> subtract to find the number of blocks in the extents. > > >> > > >> Signed-off-by: Leonard Michlmayr<leonard.michlmayr@gmail.com> > > >> Signed-off-by: "Theodore Ts'o"<tytso@mit.edu> > > >> (cherry picked from commit aca92ff6f57c000d1b4523e383c8bd6b8269b8b1 upstream) > > >> > > >> BugLink: http://launchpad.net/bugs/474597 > > >> Signed-off-by: Surbhi Palande<surbhi.palande@canonical.com> > > > Acked-by: Stefan Bader<stefan.bader@canonical.com> > > >> --- > > >> fs/ext4/extents.c | 9 +++++++-- > > >> 1 files changed, 7 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > > >> index 8b8bae4..f199d20 100644 > > >> --- a/fs/ext4/extents.c > > >> +++ b/fs/ext4/extents.c > > >> @@ -3711,7 +3711,6 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > > >> __u64 start, __u64 len) > > >> { > > >> ext4_lblk_t start_blk; > > >> - ext4_lblk_t len_blks; > > >> int error = 0; > > >> > > >> /* fallback to generic here if not in extents fmt */ > > >> @@ -3725,8 +3724,14 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > > >> if (fieinfo->fi_flags& FIEMAP_FLAG_XATTR) { > > >> error = ext4_xattr_fiemap(inode, fieinfo); > > >> } else { > > >> + ext4_lblk_t len_blks; > > >> + __u64 last_blk; > > >> + > > >> start_blk = start>> inode->i_sb->s_blocksize_bits; > > >> - len_blks = len>> inode->i_sb->s_blocksize_bits; > > >> + last_blk = (start + len - 1)>> inode->i_sb->s_blocksize_bits; > > >> + if (last_blk>= EXT_MAX_BLOCK) > > >> + last_blk = EXT_MAX_BLOCK-1; > > >> + len_blks = ((ext4_lblk_t) last_blk) - start_blk + 1; > > >> > > >> /* > > >> * Walk the extent tree gathering extent information. > > > > > > > > > > Maybe I'm misreading this bug according to > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/474597/comments/7 > > ted applied a different patch to fix this issue. > > > Brad, thanks for pointing this out. > > The patch that actually got into the linux kernel does seem to have the > additional check (missing from whats on launchpad) > +if (last_blk>= EXT_MAX_BLOCK) > + last_blk = EXT_MAX_BLOCK-1; > This check seems necessary for the corner case. > > I have pointed the difference out on the buglink too. That implies that there is a better patch out there. Am I expecting that patch to be posted instead here? -apw
Hi Folks, I was not clear enough about this patch before. The patch that I picked was cherry picked from upstream and is the correct patch. The one on launchpad was obsolete and I have not picked that one. Please do consider this patch (cherry picked from upstream) for Lucid. Thanks! Warm Regards, Surbhi. On 04/01/2010 03:44 PM, Surbhi Palande wrote: > On Thu, 2010-04-01 at 07:53 -0700, Brad Figg wrote: >> On 04/01/2010 02:37 AM, Stefan Bader wrote: >>> Ok, looks good to me now. >>> >>> Surbhi Palande wrote: >>>> The following cherry-picked patch fixes the calculation of extents in fiemap. >>>> Without this patch a user of fiemap could be misled into thinking that there >>>> are no extents in a file when actually there are. This patch is necessary for >>>> code such as ureadahead which uses fiemap to find out if the file extents have >>>> increased. >>>> >>>> The commit id in the upstream code is: aca92ff6f57c000d1b4523e383c8bd6b8269b8b1 >>>> This patch fixes bug 474597 on LP. Do consider merging this in Lucid. >>>> >>>> From dfaa33f0502a8bf9dff0e596783a40ce330c7a9b Mon Sep 17 00:00:00 2001 >>>> From: Leonard Michlmayr<leonard.michlmayr@gmail.com> >>>> Date: Thu, 4 Mar 2010 17:07:28 -0500 >>>> Subject: [PATCH] ext4: correctly calculate number of blocks for fiemap >>>> >>>> ext4_fiemap() rounds the length of the requested range down to >>>> blocksize, which is is not the true number of blocks that cover the >>>> requested region. This problem is especially impressive if the user >>>> requests only the first byte of a file: not a single extent will be >>>> reported. >>>> >>>> We fix this by calculating the last block of the region and then >>>> subtract to find the number of blocks in the extents. >>>> >>>> Signed-off-by: Leonard Michlmayr<leonard.michlmayr@gmail.com> >>>> Signed-off-by: "Theodore Ts'o"<tytso@mit.edu> >>>> (cherry picked from commit aca92ff6f57c000d1b4523e383c8bd6b8269b8b1 upstream) >>>> >>>> BugLink: http://launchpad.net/bugs/474597 >>>> Signed-off-by: Surbhi Palande<surbhi.palande@canonical.com> >>> Acked-by: Stefan Bader<stefan.bader@canonical.com> >>>> --- >>>> fs/ext4/extents.c | 9 +++++++-- >>>> 1 files changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >>>> index 8b8bae4..f199d20 100644 >>>> --- a/fs/ext4/extents.c >>>> +++ b/fs/ext4/extents.c >>>> @@ -3711,7 +3711,6 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, >>>> __u64 start, __u64 len) >>>> { >>>> ext4_lblk_t start_blk; >>>> - ext4_lblk_t len_blks; >>>> int error = 0; >>>> >>>> /* fallback to generic here if not in extents fmt */ >>>> @@ -3725,8 +3724,14 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, >>>> if (fieinfo->fi_flags& FIEMAP_FLAG_XATTR) { >>>> error = ext4_xattr_fiemap(inode, fieinfo); >>>> } else { >>>> + ext4_lblk_t len_blks; >>>> + __u64 last_blk; >>>> + >>>> start_blk = start>> inode->i_sb->s_blocksize_bits; >>>> - len_blks = len>> inode->i_sb->s_blocksize_bits; >>>> + last_blk = (start + len - 1)>> inode->i_sb->s_blocksize_bits; >>>> + if (last_blk>= EXT_MAX_BLOCK) >>>> + last_blk = EXT_MAX_BLOCK-1; >>>> + len_blks = ((ext4_lblk_t) last_blk) - start_blk + 1; >>>> >>>> /* >>>> * Walk the extent tree gathering extent information. >>> >>> >> >> Maybe I'm misreading this bug according to >> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/474597/comments/7 >> ted applied a different patch to fix this issue. >> > Brad, thanks for pointing this out. > > The patch that actually got into the linux kernel does seem to have the > additional check (missing from whats on launchpad) > +if (last_blk>= EXT_MAX_BLOCK) > + last_blk = EXT_MAX_BLOCK-1; > This check seems necessary for the corner case. > > I have pointed the difference out on the buglink too. > > Warm Regards, > Surbhi. > >> Brad > > >
Surbhi, the patch will now hit after release so the bug will need the SRU justification filling in and the right people subscribing etc. Thanks! Applied to Lucid. -apw
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 8b8bae4..f199d20 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -3711,7 +3711,6 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, __u64 start, __u64 len) { ext4_lblk_t start_blk; - ext4_lblk_t len_blks; int error = 0; /* fallback to generic here if not in extents fmt */ @@ -3725,8 +3724,14 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) { error = ext4_xattr_fiemap(inode, fieinfo); } else { + ext4_lblk_t len_blks; + __u64 last_blk; + start_blk = start >> inode->i_sb->s_blocksize_bits; - len_blks = len >> inode->i_sb->s_blocksize_bits; + last_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits; + if (last_blk >= EXT_MAX_BLOCK) + last_blk = EXT_MAX_BLOCK-1; + len_blks = ((ext4_lblk_t) last_blk) - start_blk + 1; /* * Walk the extent tree gathering extent information.