Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

2011-09-19 Thread Andi Kleen 
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
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

2011-09-19 Thread Chris Mason 
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
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

2011-09-18 Thread Jeff Liu 
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@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

2011-09-18 Thread 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.
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

2011-09-18 Thread Jeff liu 

在 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

--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

2011-09-18 Thread Chris Mason 
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
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

2011-09-17 Thread Jeff Liu 
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 a...@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 a...@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 majord...@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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

2011-09-17 Thread Andreas Dilger 
On 2011-09-17, at 12:10 AM, Jeff Liu jeff@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 a...@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 a...@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 majord...@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 majord...@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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

2011-09-17 Thread Andi Kleen 
  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-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

2011-09-16 Thread Christoph Hellwig 
On Thu, Sep 15, 2011 at 04:06:47PM -0700, Andi Kleen wrote:
 From: Andi Kleen a...@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 a...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

2011-09-16 Thread Andi Kleen 
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 a...@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
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html