Christoph Hellwig
2011-Sep-16 15:48 UTC
Re: [PATCH 1/7] BTRFS: Fix lseek return value for error
On Thu, Sep 15, 2011 at 04:06:47PM -0700, Andi Kleen wrote:> From: Andi Kleen <ak@linux.intel.com> > > Introduced by 9a4327ca1f45f82edad7dc0a4e52ce9316e0950cI think this should go to Chris/Linus ASAP. But a slightly better patch description wouldn''t hurt either. Also any reason you captialize BTRFS?> > Signed-off-by: Andi Kleen <ak@linux.intel.com> > --- > fs/btrfs/file.c | 13 +++++++------ > 1 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > index 3c3abff..7ec0a24 100644 > --- a/fs/btrfs/file.c > +++ b/fs/btrfs/file.c > @@ -1818,19 +1818,17 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin) > case SEEK_DATA: > case SEEK_HOLE: > ret = find_desired_extent(inode, &offset, origin); > - if (ret) { > - mutex_unlock(&inode->i_mutex); > - return ret; > - } > + if (ret) > + goto error; > } > > if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) { > ret = -EINVAL; > - goto out; > + goto error; > } > if (offset > inode->i_sb->s_maxbytes) { > ret = -EINVAL; > - goto out; > + goto error; > } > > /* Special lock needed here? */ > @@ -1841,6 +1839,9 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin) > out: > mutex_unlock(&inode->i_mutex); > return offset; > +error: > + mutex_unlock(&inode->i_mutex); > + return ret; > } > > const struct file_operations btrfs_file_operations = { > -- > 1.7.4.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html---end quoted text--- -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 16, 2011 at 11:48:15AM -0400, Christoph Hellwig wrote:> On Thu, Sep 15, 2011 at 04:06:47PM -0700, Andi Kleen wrote: > > From: Andi Kleen <ak@linux.intel.com> > > > > Introduced by 9a4327ca1f45f82edad7dc0a4e52ce9316e0950c > > I think this should go to Chris/Linus ASAP. But a slightly better > patch description wouldn''t hurt either.Josef acked it earlier, but with a missing Chris the btrfs merge pipeline seems to be disfunctional at the moment. -Andi
I once posted a similar patch for this issue which can be found at:
http://www.spinics.net/lists/linux-btrfs/msg12169.html
with an additional improvement if the offset is larger or equal to the
file size, return -ENXIO in directly:
if (offset >= inode->i_size) {
mutex_unlock(&inode->i_mutex);
return -ENXIO;
}
Thanks,
-Jeff
On 09/16/2011 11:48 PM, Christoph Hellwig wrote:
> On Thu, Sep 15, 2011 at 04:06:47PM -0700, Andi Kleen wrote:
>> From: Andi Kleen <ak@linux.intel.com>
>>
>> Introduced by 9a4327ca1f45f82edad7dc0a4e52ce9316e0950c
>
> I think this should go to Chris/Linus ASAP. But a slightly better
> patch description wouldn''t hurt either.
>
> Also any reason you captialize BTRFS?
>
>>
>> Signed-off-by: Andi Kleen <ak@linux.intel.com>
>> ---
>> fs/btrfs/file.c | 13 +++++++------
>> 1 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>> index 3c3abff..7ec0a24 100644
>> --- a/fs/btrfs/file.c
>> +++ b/fs/btrfs/file.c
>> @@ -1818,19 +1818,17 @@ static loff_t btrfs_file_llseek(struct file
*file, loff_t offset, int origin)
>> case SEEK_DATA:
>> case SEEK_HOLE:
>> ret = find_desired_extent(inode, &offset, origin);
>> - if (ret) {
>> - mutex_unlock(&inode->i_mutex);
>> - return ret;
>> - }
>> + if (ret)
>> + goto error;
>> }
>>
>> if (offset < 0 && !(file->f_mode &
FMODE_UNSIGNED_OFFSET)) {
>> ret = -EINVAL;
>> - goto out;
>> + goto error;
>> }
>> if (offset > inode->i_sb->s_maxbytes) {
>> ret = -EINVAL;
>> - goto out;
>> + goto error;
>> }
>>
>> /* Special lock needed here? */
>> @@ -1841,6 +1839,9 @@ static loff_t btrfs_file_llseek(struct file
*file, loff_t offset, int origin)
>> out:
>> mutex_unlock(&inode->i_mutex);
>> return offset;
>> +error:
>> + mutex_unlock(&inode->i_mutex);
>> + return ret;
>> }
>>
>> const struct file_operations btrfs_file_operations = {
>> --
>> 1.7.4.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
linux-fsdevel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> ---end quoted text---
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Andreas Dilger
2011-Sep-17 23:03 UTC
Re: [PATCH 1/7] BTRFS: Fix lseek return value for error
On 2011-09-17, at 12:10 AM, Jeff Liu <jeff.liu@oracle.com> wrote:> I once posted a similar patch for this issue which can be found at: > http://www.spinics.net/lists/linux-btrfs/msg12169.html > > with an additional improvement if the offset is larger or equal to the > file size, return -ENXIO in directly: > > if (offset >= inode->i_size) { > mutex_unlock(&inode->i_mutex); > return -ENXIO; > }Except that is wrong, because it would then be impossible to write sparse files.> Thanks, > -Jeff > On 09/16/2011 11:48 PM, Christoph Hellwig wrote: > >> On Thu, Sep 15, 2011 at 04:06:47PM -0700, Andi Kleen wrote: >>> From: Andi Kleen <ak@linux.intel.com> >>> >>> Introduced by 9a4327ca1f45f82edad7dc0a4e52ce9316e0950c >> >> I think this should go to Chris/Linus ASAP. But a slightly better >> patch description wouldn''t hurt either. >> >> Also any reason you captialize BTRFS? >> >>> >>> Signed-off-by: Andi Kleen <ak@linux.intel.com> >>> --- >>> fs/btrfs/file.c | 13 +++++++------ >>> 1 files changed, 7 insertions(+), 6 deletions(-) >>> >>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c >>> index 3c3abff..7ec0a24 100644 >>> --- a/fs/btrfs/file.c >>> +++ b/fs/btrfs/file.c >>> @@ -1818,19 +1818,17 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin) >>> case SEEK_DATA: >>> case SEEK_HOLE: >>> ret = find_desired_extent(inode, &offset, origin); >>> - if (ret) { >>> - mutex_unlock(&inode->i_mutex); >>> - return ret; >>> - } >>> + if (ret) >>> + goto error; >>> } >>> >>> if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) { >>> ret = -EINVAL; >>> - goto out; >>> + goto error; >>> } >>> if (offset > inode->i_sb->s_maxbytes) { >>> ret = -EINVAL; >>> - goto out; >>> + goto error; >>> } >>> >>> /* Special lock needed here? */ >>> @@ -1841,6 +1839,9 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin) >>> out: >>> mutex_unlock(&inode->i_mutex); >>> return offset; >>> +error: >>> + mutex_unlock(&inode->i_mutex); >>> + return ret; >>> } >>> >>> const struct file_operations btrfs_file_operations = { >>> -- >>> 1.7.4.4 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> ---end quoted text--- >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html-- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > with an additional improvement if the offset is larger or equal to the > > file size, return -ENXIO in directly: > > > > if (offset >= inode->i_size) { > > mutex_unlock(&inode->i_mutex); > > return -ENXIO; > > } > > Except that is wrong, because it would then be impossible to write sparse files.And also i_size must be always read with i_size_read() Anyways clearly there''s a problem in btrfs land with merging fixes in time. Is anyone collecting patches while Chris is gone? -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andreas and Andi, Thanks for your comments. On 09/18/2011 09:46 AM, Andi Kleen wrote:>>> with an additional improvement if the offset is larger or equal to the >>> file size, return -ENXIO in directly: >>> >>> if (offset >= inode->i_size) { >>> mutex_unlock(&inode->i_mutex); >>> return -ENXIO; >>> } >> >> Except that is wrong, because it would then be impossible to write sparse files.Per my tryout, except that, if the offset >= source file size, call lseek(fd, offset, SEEK_DATA/SEEK_HOLE) against Btrfs will always return the total file size rather than -ENXIO. however, our desired result it -ENXIO in this case, Am I right?> > And also i_size must be always read with i_size_read()Thanks for pointing this out! Would you please kindly review the revised as below? Signed-off-by: Jie Liu <jeff.liu@oracle.com> --- fs/btrfs/file.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index e7872e4..40c1ef3 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -1813,6 +1813,11 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin) goto out; case SEEK_DATA: case SEEK_HOLE: + if (offset >= i_size_read(inode)) { + mutex_unlock(&inode->i_mutex); + return -ENXIO; + } + ret = find_desired_extent(inode, &offset, origin); if (ret) { mutex_unlock(&inode->i_mutex); @@ -1821,11 +1826,11 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin) } if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) { - ret = -EINVAL; + offset = -EINVAL; goto out; } if (offset > inode->i_sb->s_maxbytes) { - ret = -EINVAL; + offset = -EINVAL; goto out; } -- 1.7.4.1 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Marco Stornelli
2011-Sep-18 08:42 UTC
Re: [PATCH 1/7] BTRFS: Fix lseek return value for error
Il 18/09/2011 09:29, Jeff Liu ha scritto:> Hi Andreas and Andi, > > Thanks for your comments. > > On 09/18/2011 09:46 AM, Andi Kleen wrote: > >>>> with an additional improvement if the offset is larger or equal to the >>>> file size, return -ENXIO in directly: >>>> >>>> if (offset>= inode->i_size) { >>>> mutex_unlock(&inode->i_mutex); >>>> return -ENXIO; >>>> } >>> >>> Except that is wrong, because it would then be impossible to write sparse files. > > Per my tryout, except that, if the offset>= source file size, call > lseek(fd, offset, SEEK_DATA/SEEK_HOLE) against Btrfs will always return > the total file size rather than -ENXIO. however, our desired result it > -ENXIO in this case, Am I right? >Yes, ENXIO should be the operation result.
在 2011-9-18，下午4:42， Marco Stornelli 写道：> Il 18/09/2011 09:29, Jeff Liu ha scritto: >> Hi Andreas and Andi, >> >> Thanks for your comments. >> >> On 09/18/2011 09:46 AM, Andi Kleen wrote: >> >>>>> with an additional improvement if the offset is larger or equal to the >>>>> file size, return -ENXIO in directly: >>>>> >>>>> if (offset>= inode->i_size) { >>>>> mutex_unlock(&inode->i_mutex); >>>>> return -ENXIO; >>>>> } >>>> >>>> Except that is wrong, because it would then be impossible to write sparse files. >> >> Per my tryout, except that, if the offset>= source file size, call >> lseek(fd, offset, SEEK_DATA/SEEK_HOLE) against Btrfs will always return >> the total file size rather than -ENXIO. however, our desired result it >> -ENXIO in this case, Am I right? >> > > Yes, ENXIO should be the operation result.Thanks for your kind confirmation. -Jeff
Excerpts from Jeff liu''s message of 2011-09-18 06:33:38 -0400:> > å¨ 2011-9-18ï¼ä¸å4:42ï¼ Marco Stornelli åéï¼ > > > Il 18/09/2011 09:29, Jeff Liu ha scritto: > >> Hi Andreas and Andi, > >> > >> Thanks for your comments. > >> > >> On 09/18/2011 09:46 AM, Andi Kleen wrote: > >> > >>>>> with an additional improvement if the offset is larger or equal to the > >>>>> file size, return -ENXIO in directly: > >>>>> > >>>>> if (offset>= inode->i_size) { > >>>>> mutex_unlock(&inode->i_mutex); > >>>>> return -ENXIO; > >>>>> } > >>>> > >>>> Except that is wrong, because it would then be impossible to write sparse files. > >> > >> Per my tryout, except that, if the offset>= source file size, call > >> lseek(fd, offset, SEEK_DATA/SEEK_HOLE) against Btrfs will always return > >> the total file size rather than -ENXIO. however, our desired result it > >> -ENXIO in this case, Am I right? > >> > > > > Yes, ENXIO should be the operation result. > > Thanks for your kind confirmation.Thanks everyone, I''ve put Jeff''s last version of this in my queue. -chris
> Thanks everyone, I''ve put Jeff''s last version of this in my queue.Can you post the version you merged? The previous ones all had issues. -Andi
Excerpts from Andi Kleen''s message of 2011-09-19 13:52:03 -0400:> > Thanks everyone, I''ve put Jeff''s last version of this in my queue. > > Can you post the version you merged? The previous ones all had issues.https://github.com/chrismason/linux/commit/48802c8ae2a9d618ec734a61283d645ad527e06c This was the last one sent, I thought it combined all the fixes. commit 48802c8ae2a9d618ec734a61283d645ad527e06c Author: Jeff Liu <jeff.liu@oracle.com> Date: Sun Sep 18 10:34:02 2011 -0400 BTRFS: Fix lseek return value for error The recent reworking of btrfs'' lseek lead to incorrect values being returned. This adds checks for seeking beyond EOF in SEEK_HOLE and makes sure the error values come back correct. Andi Kleen also sent in similar patches. Signed-off-by: Jie Liu <jeff.liu@oracle.com> Reported-by: Andi Kleen <ak@linux.intel.com> Signed-off-by: Chris Mason <chris.mason@oracle.com> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index 3c3abff..a381cd2 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -1817,6 +1817,11 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin) goto out; case SEEK_DATA: case SEEK_HOLE: + if (offset >= i_size_read(inode)) { + mutex_unlock(&inode->i_mutex); + return -ENXIO; + } + ret = find_desired_extent(inode, &offset, origin); if (ret) { mutex_unlock(&inode->i_mutex); @@ -1825,11 +1830,11 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin) } if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) { - ret = -EINVAL; + offset = -EINVAL; goto out; } if (offset > inode->i_sb->s_maxbytes) { - ret = -EINVAL; + offset = -EINVAL; goto out; } -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 19, 2011 at 03:30:02PM -0400, Chris Mason wrote:> Excerpts from Andi Kleen''s message of 2011-09-19 13:52:03 -0400: > > > Thanks everyone, I''ve put Jeff''s last version of this in my queue. > > > > Can you post the version you merged? The previous ones all had issues. > > https://github.com/chrismason/linux/commit/48802c8ae2a9d618ec734a61283d645ad527e06c > > This was the last one sent, I thought it combined all the fixes.Ok looks good, but it will be all obsolete once my patchkit lseek get in (except for the SEEK_DATA/HOLE hunk) -Andi
Excerpts from Andi Kleen''s message of 2011-09-19 15:59:52 -0400:> On Mon, Sep 19, 2011 at 03:30:02PM -0400, Chris Mason wrote: > > Excerpts from Andi Kleen''s message of 2011-09-19 13:52:03 -0400: > > > > Thanks everyone, I''ve put Jeff''s last version of this in my queue. > > > > > > Can you post the version you merged? The previous ones all had issues. > > > > https://github.com/chrismason/linux/commit/48802c8ae2a9d618ec734a61283d645ad527e06c > > > > This was the last one sent, I thought it combined all the fixes. > > Ok looks good, but it will be all obsolete once my patchkit lseek > get in (except for the SEEK_DATA/HOLE hunk)Yeah, it''s similar to yours except for the hole hunk. Your gotos are fine by me, whatever makes your patch cleaner. -chris