from:"Anton Altaparmakov"

Hi Al,

> On 29 Sep 2016, at 13:17, Al Viro <v...@zeniv.linux.org.uk> wrote:
> 
> On Thu, Sep 29, 2016 at 11:53:21AM +, Anton Altaparmakov wrote:
>> Thus if the events happen in this order:
>> 
>> evict_inodes()   iget_locked() in 
>> find_inode_fast()
> 
> ... you are buggered, because somebody is trying to grab a reference
> to inode on a filesystem that is being shut down.  Look at evict_inode()
> caller...

But what if that somebody is simply the file system being shutdown trying to 
flush some dirty metadata to disk which is stored in a file and thus accessed 
via an inode and thus iget on the inode is needed?  Surely that is allowed even 
during shutdown.  Once the write is complete iput() is called which then 
immediately evicts the inode as MS_ACTIVE is clear...

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

Re: Race condition between iget_locked() and evict_inodes()

Hi Al,

> On 29 Sep 2016, at 13:17, Al Viro  wrote:
> 
> On Thu, Sep 29, 2016 at 11:53:21AM +0000, Anton Altaparmakov wrote:
>> Thus if the events happen in this order:
>> 
>> evict_inodes()   iget_locked() in 
>> find_inode_fast()
> 
> ... you are buggered, because somebody is trying to grab a reference
> to inode on a filesystem that is being shut down.  Look at evict_inode()
> caller...

But what if that somebody is simply the file system being shutdown trying to 
flush some dirty metadata to disk which is stored in a file and thus accessed 
via an inode and thus iget on the inode is needed?  Surely that is allowed even 
during shutdown.  Once the write is complete iput() is called which then 
immediately evicts the inode as MS_ACTIVE is clear...

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

Race condition between iget_locked() and evict_inodes()

Hi Al,

I think there is a race condition between iget_locked() and evict_inodes().

evict_inodes() checks i_count and if zero proceeds to take i_lock then set 
I_FREEING and eventually disposes of the inode.

But a concurrent iget_locked() takes i_lock and then increments i_count.

Thus if the events happen in this order:

evict_inodes()  iget_locked() in find_inode_fast()
atomic_read(>i_count) -> 0   take i_lock
wait on i_lock  __iget(inode); -> i_count now 1
set I_FREEING   drop i_lock
evict() return inode to caller

The inode is now gone due to the evict() call whilst it is happily being used 
with an elevated i_count by the iget_locked() calling process.  It seems to me 
like evict_inodes() should be checking i_count inside i_lock either as the only 
check or it should at least re-check it.

Please tell me what I am missing here.  I assume there must be something 
providing exclusion and I am just too blind to see it but I thought it worth 
bringing to your attention in case it really is simply broken.

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

Race condition between iget_locked() and evict_inodes()

Hi Al,

I think there is a race condition between iget_locked() and evict_inodes().

evict_inodes() checks i_count and if zero proceeds to take i_lock then set 
I_FREEING and eventually disposes of the inode.

But a concurrent iget_locked() takes i_lock and then increments i_count.

Thus if the events happen in this order:

evict_inodes()  iget_locked() in find_inode_fast()
atomic_read(>i_count) -> 0   take i_lock
wait on i_lock  __iget(inode); -> i_count now 1
set I_FREEING   drop i_lock
evict() return inode to caller

The inode is now gone due to the evict() call whilst it is happily being used 
with an elevated i_count by the iget_locked() calling process.  It seems to me 
like evict_inodes() should be checking i_count inside i_lock either as the only 
check or it should at least re-check it.

Please tell me what I am missing here.  I assume there must be something 
providing exclusion and I am just too blind to see it but I thought it worth 
bringing to your attention in case it really is simply broken.

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation


> On 24 Sep 2015, at 10:20, Anton Altaparmakov  wrote:
> 
> Hi Andrzej,
> 
>> On 24 Sep 2015, at 09:34, Andrzej Hajda  wrote:
>> 
>> On 09/23/2015 12:21 PM, Anton Altaparmakov wrote:
>>> Hi Andrzej,
>>> 
>>> Thanks for your patch.  It looks fine though I don't quite see the point of 
>>> it to be honest.
>>> 
>>> It actually adds an additional function call (kmemdup() is not inline) just 
>>> to save 1 line of source code in the driver and I don't think it improves 
>>> readability or anything so why bother?  What does it gain?
>> 
>> kmemdup replaces combo (kmalloc + memdup) with one call.
>> The patch follows quite common practice of abstracting out common patterns.
> 
> Sure it does I am just questioning the sanity of the practice...  (-;
> 
> Such changes reduce the size of the kernel binary by a few bytes at the cost 
> of adding CPU cycles to the execution time.  How is that good thing?  Unless 
> you are on an embedded system desperate for RAM throwing away CPU cycles on 
> pointless abstractions makes no sense to me...
> 
> But as I said patch is fine.  Feel free to send it onto Andrew to get it into 
> mainline.  You can add my Acked-by: Anton Altaparmakoc 

Altaparmakov even.  Can't even spell my own surname...  )-;

> line to it when sending it.  I am just saying that I think patches like that 
> don't make much sense to me...
> 
> Best regards,
> 
>   Anton
> 
>> Regards
>> Andrzej
>> 
>>> 
>>> Best regards,
>>> 
>>> Anton
>>> 
>>>> On 7 Aug 2015, at 08:59, Andrzej Hajda  wrote:
>>>> 
>>>> The patch was generated using fixed coccinelle semantic patch
>>>> scripts/coccinelle/api/memdup.cocci [1].
>>>> 
>>>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2014320
>>>> 
>>>> Signed-off-by: Andrzej Hajda 
>>>> ---
>>>> fs/ntfs/dir.c | 7 +++
>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>> 
>>>> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
>>>> index 9e38daf..2b7fef0 100644
>>>> --- a/fs/ntfs/dir.c
>>>> +++ b/fs/ntfs/dir.c
>>>> @@ -1172,14 +1172,13 @@ static int ntfs_readdir(struct file *file, struct 
>>>> dir_context *actor)
>>>> * map the mft record without deadlocking.
>>>> */
>>>>rc = le32_to_cpu(ctx->attr->data.resident.value_length);
>>>> -  ir = kmalloc(rc, GFP_NOFS);
>>>> +  /* Copy the index root value (it has been verified in read_inode). */
>>>> +  ir = kmemdup((u8 *)ctx->attr + 
>>>> le16_to_cpu(ctx->attr->data.resident.value_offset),
>>>> +   rc, GFP_NOFS);
>>>>if (unlikely(!ir)) {
>>>>err = -ENOMEM;
>>>>goto err_out;
>>>>}
>>>> -  /* Copy the index root value (it has been verified in read_inode). */
>>>> -  memcpy(ir, (u8*)ctx->attr +
>>>> -  le16_to_cpu(ctx->attr->data.resident.value_offset), rc);
>>>>ntfs_attr_put_search_ctx(ctx);
>>>>unmap_mft_record(ndir);
>>>>ctx = NULL;
>>>> -- 
>>>> 1.9.1
> 
> -- 
> Anton Altaparmakov  (replace at with @)
> Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
> Linux NTFS maintainer

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation

Hi Andrzej,

> On 24 Sep 2015, at 09:34, Andrzej Hajda  wrote:
> 
> On 09/23/2015 12:21 PM, Anton Altaparmakov wrote:
>> Hi Andrzej,
>> 
>> Thanks for your patch.  It looks fine though I don't quite see the point of 
>> it to be honest.
>> 
>> It actually adds an additional function call (kmemdup() is not inline) just 
>> to save 1 line of source code in the driver and I don't think it improves 
>> readability or anything so why bother?  What does it gain?
> 
> kmemdup replaces combo (kmalloc + memdup) with one call.
> The patch follows quite common practice of abstracting out common patterns.

Sure it does I am just questioning the sanity of the practice...  (-;

Such changes reduce the size of the kernel binary by a few bytes at the cost of 
adding CPU cycles to the execution time.  How is that good thing?  Unless you 
are on an embedded system desperate for RAM throwing away CPU cycles on 
pointless abstractions makes no sense to me...

But as I said patch is fine.  Feel free to send it onto Andrew to get it into 
mainline.  You can add my Acked-by: Anton Altaparmakoc  line 
to it when sending it.  I am just saying that I think patches like that don't 
make much sense to me...

Best regards,

Anton

> Regards
> Andrzej
> 
>> 
>> Best regards,
>> 
>>  Anton
>> 
>>> On 7 Aug 2015, at 08:59, Andrzej Hajda  wrote:
>>> 
>>> The patch was generated using fixed coccinelle semantic patch
>>> scripts/coccinelle/api/memdup.cocci [1].
>>> 
>>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2014320
>>> 
>>> Signed-off-by: Andrzej Hajda 
>>> ---
>>> fs/ntfs/dir.c | 7 +++
>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
>>> index 9e38daf..2b7fef0 100644
>>> --- a/fs/ntfs/dir.c
>>> +++ b/fs/ntfs/dir.c
>>> @@ -1172,14 +1172,13 @@ static int ntfs_readdir(struct file *file, struct 
>>> dir_context *actor)
>>>  * map the mft record without deadlocking.
>>>  */
>>> rc = le32_to_cpu(ctx->attr->data.resident.value_length);
>>> -   ir = kmalloc(rc, GFP_NOFS);
>>> +   /* Copy the index root value (it has been verified in read_inode). */
>>> +   ir = kmemdup((u8 *)ctx->attr + 
>>> le16_to_cpu(ctx->attr->data.resident.value_offset),
>>> +rc, GFP_NOFS);
>>> if (unlikely(!ir)) {
>>>     err = -ENOMEM;
>>> goto err_out;
>>> }
>>> -   /* Copy the index root value (it has been verified in read_inode). */
>>> -   memcpy(ir, (u8*)ctx->attr +
>>> -   le16_to_cpu(ctx->attr->data.resident.value_offset), rc);
>>> ntfs_attr_put_search_ctx(ctx);
>>> unmap_mft_record(ndir);
>>> ctx = NULL;
>>> -- 
>>> 1.9.1

-- 
Anton Altaparmakov  (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation


> On 24 Sep 2015, at 10:20, Anton Altaparmakov <an...@tuxera.com> wrote:
> 
> Hi Andrzej,
> 
>> On 24 Sep 2015, at 09:34, Andrzej Hajda <a.ha...@samsung.com> wrote:
>> 
>> On 09/23/2015 12:21 PM, Anton Altaparmakov wrote:
>>> Hi Andrzej,
>>> 
>>> Thanks for your patch.  It looks fine though I don't quite see the point of 
>>> it to be honest.
>>> 
>>> It actually adds an additional function call (kmemdup() is not inline) just 
>>> to save 1 line of source code in the driver and I don't think it improves 
>>> readability or anything so why bother?  What does it gain?
>> 
>> kmemdup replaces combo (kmalloc + memdup) with one call.
>> The patch follows quite common practice of abstracting out common patterns.
> 
> Sure it does I am just questioning the sanity of the practice...  (-;
> 
> Such changes reduce the size of the kernel binary by a few bytes at the cost 
> of adding CPU cycles to the execution time.  How is that good thing?  Unless 
> you are on an embedded system desperate for RAM throwing away CPU cycles on 
> pointless abstractions makes no sense to me...
> 
> But as I said patch is fine.  Feel free to send it onto Andrew to get it into 
> mainline.  You can add my Acked-by: Anton Altaparmakoc <an...@tuxera.com>

Altaparmakov even.  Can't even spell my own surname...  )-;

> line to it when sending it.  I am just saying that I think patches like that 
> don't make much sense to me...
> 
> Best regards,
> 
>   Anton
> 
>> Regards
>> Andrzej
>> 
>>> 
>>> Best regards,
>>> 
>>> Anton
>>> 
>>>> On 7 Aug 2015, at 08:59, Andrzej Hajda <a.ha...@samsung.com> wrote:
>>>> 
>>>> The patch was generated using fixed coccinelle semantic patch
>>>> scripts/coccinelle/api/memdup.cocci [1].
>>>> 
>>>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2014320
>>>> 
>>>> Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
>>>> ---
>>>> fs/ntfs/dir.c | 7 +++
>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>> 
>>>> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
>>>> index 9e38daf..2b7fef0 100644
>>>> --- a/fs/ntfs/dir.c
>>>> +++ b/fs/ntfs/dir.c
>>>> @@ -1172,14 +1172,13 @@ static int ntfs_readdir(struct file *file, struct 
>>>> dir_context *actor)
>>>> * map the mft record without deadlocking.
>>>> */
>>>>rc = le32_to_cpu(ctx->attr->data.resident.value_length);
>>>> -  ir = kmalloc(rc, GFP_NOFS);
>>>> +  /* Copy the index root value (it has been verified in read_inode). */
>>>> +  ir = kmemdup((u8 *)ctx->attr + 
>>>> le16_to_cpu(ctx->attr->data.resident.value_offset),
>>>> +   rc, GFP_NOFS);
>>>>if (unlikely(!ir)) {
>>>>err = -ENOMEM;
>>>>goto err_out;
>>>>}
>>>> -  /* Copy the index root value (it has been verified in read_inode). */
>>>> -  memcpy(ir, (u8*)ctx->attr +
>>>> -  le16_to_cpu(ctx->attr->data.resident.value_offset), rc);
>>>>ntfs_attr_put_search_ctx(ctx);
>>>>unmap_mft_record(ndir);
>>>>ctx = NULL;
>>>> -- 
>>>> 1.9.1
> 
> -- 
> Anton Altaparmakov  (replace at with @)
> Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
> Linux NTFS maintainer

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation

Hi Andrzej,

> On 24 Sep 2015, at 09:34, Andrzej Hajda <a.ha...@samsung.com> wrote:
> 
> On 09/23/2015 12:21 PM, Anton Altaparmakov wrote:
>> Hi Andrzej,
>> 
>> Thanks for your patch.  It looks fine though I don't quite see the point of 
>> it to be honest.
>> 
>> It actually adds an additional function call (kmemdup() is not inline) just 
>> to save 1 line of source code in the driver and I don't think it improves 
>> readability or anything so why bother?  What does it gain?
> 
> kmemdup replaces combo (kmalloc + memdup) with one call.
> The patch follows quite common practice of abstracting out common patterns.

Sure it does I am just questioning the sanity of the practice...  (-;

Such changes reduce the size of the kernel binary by a few bytes at the cost of 
adding CPU cycles to the execution time.  How is that good thing?  Unless you 
are on an embedded system desperate for RAM throwing away CPU cycles on 
pointless abstractions makes no sense to me...

But as I said patch is fine.  Feel free to send it onto Andrew to get it into 
mainline.  You can add my Acked-by: Anton Altaparmakoc <an...@tuxera.com> line 
to it when sending it.  I am just saying that I think patches like that don't 
make much sense to me...

Best regards,

Anton

> Regards
> Andrzej
> 
>> 
>> Best regards,
>> 
>>  Anton
>> 
>>> On 7 Aug 2015, at 08:59, Andrzej Hajda <a.ha...@samsung.com> wrote:
>>> 
>>> The patch was generated using fixed coccinelle semantic patch
>>> scripts/coccinelle/api/memdup.cocci [1].
>>> 
>>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2014320
>>> 
>>> Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
>>> ---
>>> fs/ntfs/dir.c | 7 +++
>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
>>> index 9e38daf..2b7fef0 100644
>>> --- a/fs/ntfs/dir.c
>>> +++ b/fs/ntfs/dir.c
>>> @@ -1172,14 +1172,13 @@ static int ntfs_readdir(struct file *file, struct 
>>> dir_context *actor)
>>>  * map the mft record without deadlocking.
>>>  */
>>> rc = le32_to_cpu(ctx->attr->data.resident.value_length);
>>> -   ir = kmalloc(rc, GFP_NOFS);
>>> +   /* Copy the index root value (it has been verified in read_inode). */
>>> +   ir = kmemdup((u8 *)ctx->attr + 
>>> le16_to_cpu(ctx->attr->data.resident.value_offset),
>>> +rc, GFP_NOFS);
>>> if (unlikely(!ir)) {
>>> err = -ENOMEM;
>>> goto err_out;
>>> }
>>> -   /* Copy the index root value (it has been verified in read_inode). */
>>> -   memcpy(ir, (u8*)ctx->attr +
>>> -   le16_to_cpu(ctx->attr->data.resident.value_offset), rc);
>>> ntfs_attr_put_search_ctx(ctx);
>>> unmap_mft_record(ndir);
>>> ctx = NULL;
>>> -- 
>>> 1.9.1

-- 
Anton Altaparmakov  (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation

2015-09-23 Thread Anton Altaparmakov

Hi Andrzej,

Thanks for your patch.  It looks fine though I don't quite see the point of it 
to be honest.

It actually adds an additional function call (kmemdup() is not inline) just to 
save 1 line of source code in the driver and I don't think it improves 
readability or anything so why bother?  What does it gain?

Best regards,

Anton

> On 7 Aug 2015, at 08:59, Andrzej Hajda  wrote:
> 
> The patch was generated using fixed coccinelle semantic patch
> scripts/coccinelle/api/memdup.cocci [1].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2014320
> 
> Signed-off-by: Andrzej Hajda 
> ---
> fs/ntfs/dir.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
> index 9e38daf..2b7fef0 100644
> --- a/fs/ntfs/dir.c
> +++ b/fs/ntfs/dir.c
> @@ -1172,14 +1172,13 @@ static int ntfs_readdir(struct file *file, struct 
> dir_context *actor)
>* map the mft record without deadlocking.
>*/
>   rc = le32_to_cpu(ctx->attr->data.resident.value_length);
> - ir = kmalloc(rc, GFP_NOFS);
> + /* Copy the index root value (it has been verified in read_inode). */
> + ir = kmemdup((u8 *)ctx->attr + 
> le16_to_cpu(ctx->attr->data.resident.value_offset),
> +  rc, GFP_NOFS);
>   if (unlikely(!ir)) {
>   err = -ENOMEM;
>   goto err_out;
>   }
> - /* Copy the index root value (it has been verified in read_inode). */
> - memcpy(ir, (u8*)ctx->attr +
> - le16_to_cpu(ctx->attr->data.resident.value_offset), rc);
>   ntfs_attr_put_search_ctx(ctx);
>   unmap_mft_record(ndir);
>   ctx = NULL;
> -- 
> 1.9.1

-- 
Anton Altaparmakov  (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation

2015-09-23 Thread Anton Altaparmakov

Hi Andrzej,

Thanks for your patch.  It looks fine though I don't quite see the point of it 
to be honest.

It actually adds an additional function call (kmemdup() is not inline) just to 
save 1 line of source code in the driver and I don't think it improves 
readability or anything so why bother?  What does it gain?

Best regards,

Anton

> On 7 Aug 2015, at 08:59, Andrzej Hajda <a.ha...@samsung.com> wrote:
> 
> The patch was generated using fixed coccinelle semantic patch
> scripts/coccinelle/api/memdup.cocci [1].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2014320
> 
> Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
> ---
> fs/ntfs/dir.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
> index 9e38daf..2b7fef0 100644
> --- a/fs/ntfs/dir.c
> +++ b/fs/ntfs/dir.c
> @@ -1172,14 +1172,13 @@ static int ntfs_readdir(struct file *file, struct 
> dir_context *actor)
>* map the mft record without deadlocking.
>*/
>   rc = le32_to_cpu(ctx->attr->data.resident.value_length);
> - ir = kmalloc(rc, GFP_NOFS);
> + /* Copy the index root value (it has been verified in read_inode). */
> + ir = kmemdup((u8 *)ctx->attr + 
> le16_to_cpu(ctx->attr->data.resident.value_offset),
> +  rc, GFP_NOFS);
>   if (unlikely(!ir)) {
>   err = -ENOMEM;
>   goto err_out;
>   }
> - /* Copy the index root value (it has been verified in read_inode). */
> - memcpy(ir, (u8*)ctx->attr +
> - le16_to_cpu(ctx->attr->data.resident.value_offset), rc);
>   ntfs_attr_put_search_ctx(ctx);
>   unmap_mft_record(ndir);
>   ctx = NULL;
> -- 
> 1.9.1

-- 
Anton Altaparmakov  (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ntfs: Deletion of unnecessary checks before the function call "iput"

2015-07-05 Thread Anton Altaparmakov

Hi Andrew,

Can you please take up this trivial patch and merge it upstream?

Reviewed-by: Anton Altaparmakov 

Thanks a lot in advance!

Best regards,

Anton

> On 4 Jul 2015, at 11:32, SF Markus Elfring  
> wrote:
> 
>> From: Markus Elfring 
>> Date: Sat, 15 Nov 2014 19:35:05 +0100
>> 
>> The iput() function tests whether its argument is NULL and then
>> returns immediately. Thus the test around the call is not needed.
>> 
>> This issue was detected by using the Coccinelle software.
>> 
>> Signed-off-by: Markus Elfring 
>> ---
>> fs/ntfs/super.c | 21 +++--
>> 1 file changed, 7 insertions(+), 14 deletions(-)
>> 
>> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
>> index 6c3296e..8f22a47 100644
>> --- a/fs/ntfs/super.c
>> +++ b/fs/ntfs/super.c
>> @@ -2204,17 +2204,12 @@ get_ctx_vol_failed:
>>  return true;
>> #ifdef NTFS_RW
>> iput_usnjrnl_err_out:
>> -if (vol->usnjrnl_j_ino)
>> -iput(vol->usnjrnl_j_ino);
>> -if (vol->usnjrnl_max_ino)
>> -iput(vol->usnjrnl_max_ino);
>> -if (vol->usnjrnl_ino)
>> -iput(vol->usnjrnl_ino);
>> +iput(vol->usnjrnl_j_ino);
>> +iput(vol->usnjrnl_max_ino);
>> +iput(vol->usnjrnl_ino);
>> iput_quota_err_out:
>> -if (vol->quota_q_ino)
>> -iput(vol->quota_q_ino);
>> -if (vol->quota_ino)
>> -iput(vol->quota_ino);
>> +iput(vol->quota_q_ino);
>> +iput(vol->quota_ino);
>>  iput(vol->extend_ino);
>> #endif /* NTFS_RW */
>> iput_sec_err_out:
>> @@ -2223,8 +2218,7 @@ iput_root_err_out:
>>  iput(vol->root_ino);
>> iput_logfile_err_out:
>> #ifdef NTFS_RW
>> -if (vol->logfile_ino)
>> -iput(vol->logfile_ino);
>> +iput(vol->logfile_ino);
>> iput_vol_err_out:
>> #endif /* NTFS_RW */
>>  iput(vol->vol_ino);
>> @@ -2254,8 +2248,7 @@ iput_mftbmp_err_out:
>>  iput(vol->mftbmp_ino);
>> iput_mirr_err_out:
>> #ifdef NTFS_RW
>> -if (vol->mftmirr_ino)
>> -iput(vol->mftmirr_ino);
>> +iput(vol->mftmirr_ino);
>> #endif /* NTFS_RW */
>>  return false;
>> }
>> 
> 
> Would you like to integrate this update suggestion
> into another source code repository?
> 
> Regards,
> Markus

-- 
Anton Altaparmakov  (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ntfs: Deletion of unnecessary checks before the function call iput

2015-07-05 Thread Anton Altaparmakov

Hi Andrew,

Can you please take up this trivial patch and merge it upstream?

Reviewed-by: Anton Altaparmakov an...@tuxera.com

Thanks a lot in advance!

Best regards,

Anton

 On 4 Jul 2015, at 11:32, SF Markus Elfring elfr...@users.sourceforge.net 
 wrote:
 
 From: Markus Elfring elfr...@users.sourceforge.net
 Date: Sat, 15 Nov 2014 19:35:05 +0100
 
 The iput() function tests whether its argument is NULL and then
 returns immediately. Thus the test around the call is not needed.
 
 This issue was detected by using the Coccinelle software.
 
 Signed-off-by: Markus Elfring elfr...@users.sourceforge.net
 ---
 fs/ntfs/super.c | 21 +++--
 1 file changed, 7 insertions(+), 14 deletions(-)
 
 diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
 index 6c3296e..8f22a47 100644
 --- a/fs/ntfs/super.c
 +++ b/fs/ntfs/super.c
 @@ -2204,17 +2204,12 @@ get_ctx_vol_failed:
  return true;
 #ifdef NTFS_RW
 iput_usnjrnl_err_out:
 -if (vol-usnjrnl_j_ino)
 -iput(vol-usnjrnl_j_ino);
 -if (vol-usnjrnl_max_ino)
 -iput(vol-usnjrnl_max_ino);
 -if (vol-usnjrnl_ino)
 -iput(vol-usnjrnl_ino);
 +iput(vol-usnjrnl_j_ino);
 +iput(vol-usnjrnl_max_ino);
 +iput(vol-usnjrnl_ino);
 iput_quota_err_out:
 -if (vol-quota_q_ino)
 -iput(vol-quota_q_ino);
 -if (vol-quota_ino)
 -iput(vol-quota_ino);
 +iput(vol-quota_q_ino);
 +iput(vol-quota_ino);
  iput(vol-extend_ino);
 #endif /* NTFS_RW */
 iput_sec_err_out:
 @@ -2223,8 +2218,7 @@ iput_root_err_out:
  iput(vol-root_ino);
 iput_logfile_err_out:
 #ifdef NTFS_RW
 -if (vol-logfile_ino)
 -iput(vol-logfile_ino);
 +iput(vol-logfile_ino);
 iput_vol_err_out:
 #endif /* NTFS_RW */
  iput(vol-vol_ino);
 @@ -2254,8 +2248,7 @@ iput_mftbmp_err_out:
  iput(vol-mftbmp_ino);
 iput_mirr_err_out:
 #ifdef NTFS_RW
 -if (vol-mftmirr_ino)
 -iput(vol-mftmirr_ino);
 +iput(vol-mftmirr_ino);
 #endif /* NTFS_RW */
  return false;
 }
 
 
 Would you like to integrate this update suggestion
 into another source code repository?
 
 Regards,
 Markus

-- 
Anton Altaparmakov anton at tuxera.com (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

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

i_uid_read()/i_uid_write() and friends

2015-04-10 Thread Anton Altaparmakov

Hi,

Is it intended that non-gpl file systems cannot use functions like i_uid_read() 
and i_uid_write() (introduced by Eric Biederman in 3.5 kernel)?

They resolve to the below (in include/linux/fs.h):

static inline uid_t i_uid_read(const struct inode *inode)
{
return from_kuid(_user_ns, inode->i_uid);
}

static inline void i_uid_write(struct inode *inode, uid_t uid)
{
inode->i_uid = make_kuid(_user_ns, uid);
}

And both from_kuid() and make_kuid() are EXPORT_SYMBOL() so they are fine but 
the problem is that init_user_ns is EXPORT_SYMBOL_GPL() and because 
i_uid_read() and i_uid_write() are static inline it causes them to be unusable 
from non-gpl kernel modules...

Same thing applies to i_gid_read() and i_gid_write().

These seem pretty fundamental calls that a non-gpl file system should be able 
to call, no?

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

i_uid_read()/i_uid_write() and friends

2015-04-10 Thread Anton Altaparmakov

Hi,

Is it intended that non-gpl file systems cannot use functions like i_uid_read() 
and i_uid_write() (introduced by Eric Biederman in 3.5 kernel)?

They resolve to the below (in include/linux/fs.h):

static inline uid_t i_uid_read(const struct inode *inode)
{
return from_kuid(init_user_ns, inode-i_uid);
}

static inline void i_uid_write(struct inode *inode, uid_t uid)
{
inode-i_uid = make_kuid(init_user_ns, uid);
}

And both from_kuid() and make_kuid() are EXPORT_SYMBOL() so they are fine but 
the problem is that init_user_ns is EXPORT_SYMBOL_GPL() and because 
i_uid_read() and i_uid_write() are static inline it causes them to be unusable 
from non-gpl kernel modules...

Same thing applies to i_gid_read() and i_gid_write().

These seem pretty fundamental calls that a non-gpl file system should be able 
to call, no?

Best regards,

Anton
-- 
Anton Altaparmakov anton at tuxera.com (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

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

Re: [fuse-devel] [PATCH v5 7/7] add a flag for per-operation O_DSYNC semantics

Hi,

> On 7 Nov 2014, at 07:52, Anand Avati  wrote:
> On Thu, Nov 6, 2014 at 8:22 PM, Anton Altaparmakov  wrote:
> > On 7 Nov 2014, at 01:46, Jeff Moyer  wrote:
> > Minor nit, but I'd rather read something that looks like this:
> >
> >   if (type == READ && (flags & RWF_NONBLOCK))
> >   return -EAGAIN;
> >   else if (type == WRITE && (flags & RWF_DSYNC))
> >   return -EINVAL;
> 
> But your version is less logically efficient for the case where "type == 
> READ" is true and "flags & RWF_NONBLOCK" is false because your version then 
> has to do the "if (type == WRITE" check before discovering it does not need 
> to take that branch either, whilst the original version does not have to do 
> such a test at all.
> 
> Seriously?

Of course seriously.

> Just focus on the code readability/maintainability which makes the code most 
> easily understood/obvious to a new pair of eyes, and leave such 
> micro-optimizations to the compiler..

The original version is more readable (IMO) and this is not a 
micro-optimization.  It is people like you who are responsible for the fact 
that we need faster and faster computers to cope with the inefficient/poor code 
being written more and more...

And I really wouldn't hedge my bets on gcc optimizing something like that.  The 
amount of crap assembly produced from gcc that I have seen over the years 
suggests that it is quite likely it will make a hash of it instead...

Best regards,

Anton

> Thanks

-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 7/7] add a flag for per-operation O_DSYNC semantics

Hi Jeff,

> On 7 Nov 2014, at 01:46, Jeff Moyer  wrote:
> 
> Milosz Tanski  writes:
> 
>> -if (type == READ && (flags & RWF_NONBLOCK))
>> -return -EAGAIN;
>> +if (type == READ) {
>> +if (flags & RWF_NONBLOCK)
>> +return -EAGAIN;
>> +} else {
>> +if (flags & RWF_DSYNC)
>> +return -EINVAL;
>> +}
> 
> Minor nit, but I'd rather read something that looks like this:
> 
>   if (type == READ && (flags & RWF_NONBLOCK))
>   return -EAGAIN;
>   else if (type == WRITE && (flags & RWF_DSYNC))
>   return -EINVAL;

But your version is less logically efficient for the case where "type == READ" 
is true and "flags & RWF_NONBLOCK" is false because your version then has to do 
the "if (type == WRITE" check before discovering it does not need to take that 
branch either, whilst the original version does not have to do such a test at 
all.

Best regards,

Anton

> I won't lose sleep over it, though.
> 
> Reviewed-by: Jeff Moyer 

-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linux-NTFS-Dev] [PATCH v5 6/7] fs: pass iocb to generic_write_sync

t; - ssize_t err;
> -
> - err = generic_write_sync(file, iocb->ki_pos - ret, ret);
> - if (err < 0)
> - ret = err;
> - }
> + ret = generic_write_sync(iocb, ret);
>   if (o_direct)
>   blk_finish_plug();
> 
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index 80dd44d..3fafeca 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -895,8 +895,13 @@ retry:
>   gfs2_quota_unlock(ip);
>   }
> 
> - if (error == 0)
> - error = generic_write_sync(file, pos, count);
> + if (error)
> + goto out_unlock;
> +
> + if ((file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host)) {
> + error = vfs_fsync_range(file, pos, pos + count - 1,
> +(file->f_flags & __O_SYNC) ? 0 : 1);
> + }
>   goto out_unlock;
> 
> out_trans_fail:
> diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
> index 643faa4..4f3d664 100644
> --- a/fs/ntfs/file.c
> +++ b/fs/ntfs/file.c
> @@ -2127,12 +2127,8 @@ static ssize_t ntfs_file_aio_write(struct kiocb *iocb, 
> const struct iovec *iov,
>   mutex_lock(>i_mutex);
>   ret = ntfs_file_aio_write_nolock(iocb, iov, nr_segs, >ki_pos);
>   mutex_unlock(>i_mutex);
> - if (ret > 0) {
> - int err = generic_write_sync(file, iocb->ki_pos - ret, ret);
> - if (err < 0)
> - ret = err;
> - }
> - return ret;
> +
> + return generic_write_sync(iocb, ret);
> }
> 
> /**
> diff --git a/fs/udf/file.c b/fs/udf/file.c
> index bb15771..1cdabd0 100644
> --- a/fs/udf/file.c
> +++ b/fs/udf/file.c
> @@ -155,16 +155,9 @@ static ssize_t udf_file_write_iter(struct kiocb *iocb, 
> struct iov_iter *from)
>   retval = __generic_file_write_iter(iocb, from);
>   mutex_unlock(>i_mutex);
> 
> - if (retval > 0) {
> - ssize_t err;
> -
> + if (retval > 0)
>   mark_inode_dirty(inode);
> - err = generic_write_sync(file, iocb->ki_pos - retval, retval);
> - if (err < 0)
> - retval = err;
> - }
> -
> - return retval;
> + return generic_write_sync(iocb, retval);
> }
> 
> long udf_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 0655915..a8cab66 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -792,14 +792,8 @@ xfs_file_write_iter(
>   ret = xfs_file_buffered_aio_write(iocb, from);
> 
>   if (ret > 0) {
> - ssize_t err;
> -
>   XFS_STATS_ADD(xs_write_bytes, ret);
> -
> - /* Handle various SYNC-type writes */
> - err = generic_write_sync(file, iocb->ki_pos - ret, ret);
> - if (err < 0)
> - ret = err;
> + ret = generic_write_sync(iocb, ret);
>   }
>   return ret;
> }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index eaebd99..7d0e116 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2242,13 +2242,7 @@ extern int filemap_fdatawrite_range(struct 
> address_space *mapping,
> extern int vfs_fsync_range(struct file *file, loff_t start, loff_t end,
>  int datasync);
> extern int vfs_fsync(struct file *file, int datasync);
> -static inline int generic_write_sync(struct file *file, loff_t pos, loff_t 
> count)
> -{
> - if (!(file->f_flags & O_DSYNC) && !IS_SYNC(file->f_mapping->host))
> - return 0;
> - return vfs_fsync_range(file, pos, pos + count - 1,
> -(file->f_flags & __O_SYNC) ? 0 : 1);
> -}
> +extern int generic_write_sync(struct kiocb *iocb, loff_t count);
> extern void emergency_sync(void);
> extern void emergency_remount(void);
> #ifdef CONFIG_BLOCK
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 09d3af3..6107058 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2664,6 +2664,24 @@ out:
> }
> EXPORT_SYMBOL(__generic_file_write_iter);
> 
> +int generic_write_sync(struct kiocb *iocb, loff_t count)
> +{
> + struct file *file = iocb->ki_filp;
> +
> + if (count > 0 &&
> + ((file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host))) {
> + bool fdatasync = !(file->f_flags & __O_SYNC);
> + ssize_t ret = 0;

That "= 0" is pointless.  "ret" is overwritten unconditionally on the following 
line...

Other than that the NTFS bits are:

Acked-by: Anton Altaparm

Re: [PATCH v5 7/7] add a flag for per-operation O_DSYNC semantics

Hi Jeff,

 On 7 Nov 2014, at 01:46, Jeff Moyer jmo...@redhat.com wrote:
 
 Milosz Tanski mil...@adfin.com writes:
 
 -if (type == READ  (flags  RWF_NONBLOCK))
 -return -EAGAIN;
 +if (type == READ) {
 +if (flags  RWF_NONBLOCK)
 +return -EAGAIN;
 +} else {
 +if (flags  RWF_DSYNC)
 +return -EINVAL;
 +}
 
 Minor nit, but I'd rather read something that looks like this:
 
   if (type == READ  (flags  RWF_NONBLOCK))
   return -EAGAIN;
   else if (type == WRITE  (flags  RWF_DSYNC))
   return -EINVAL;

But your version is less logically efficient for the case where type == READ 
is true and flags  RWF_NONBLOCK is false because your version then has to do 
the if (type == WRITE check before discovering it does not need to take that 
branch either, whilst the original version does not have to do such a test at 
all.

Best regards,

Anton

 I won't lose sleep over it, though.
 
 Reviewed-by: Jeff Moyer jmo...@redhat.com

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [fuse-devel] [PATCH v5 7/7] add a flag for per-operation O_DSYNC semantics

Hi,

 On 7 Nov 2014, at 07:52, Anand Avati av...@gluster.org wrote:
 On Thu, Nov 6, 2014 at 8:22 PM, Anton Altaparmakov ai...@cam.ac.uk wrote:
  On 7 Nov 2014, at 01:46, Jeff Moyer jmo...@redhat.com wrote:
  Minor nit, but I'd rather read something that looks like this:
 
if (type == READ  (flags  RWF_NONBLOCK))
return -EAGAIN;
else if (type == WRITE  (flags  RWF_DSYNC))
return -EINVAL;
 
 But your version is less logically efficient for the case where type == 
 READ is true and flags  RWF_NONBLOCK is false because your version then 
 has to do the if (type == WRITE check before discovering it does not need 
 to take that branch either, whilst the original version does not have to do 
 such a test at all.
 
 Seriously?

Of course seriously.

 Just focus on the code readability/maintainability which makes the code most 
 easily understood/obvious to a new pair of eyes, and leave such 
 micro-optimizations to the compiler..

The original version is more readable (IMO) and this is not a 
micro-optimization.  It is people like you who are responsible for the fact 
that we need faster and faster computers to cope with the inefficient/poor code 
being written more and more...

And I really wouldn't hedge my bets on gcc optimizing something like that.  The 
amount of crap assembly produced from gcc that I have seen over the years 
suggests that it is quite likely it will make a hash of it instead...

Best regards,

Anton

 Thanks

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [Linux-NTFS-Dev] [PATCH v5 6/7] fs: pass iocb to generic_write_sync

)
 + goto out_unlock;
 +
 + if ((file-f_flags  O_DSYNC) || IS_SYNC(file-f_mapping-host)) {
 + error = vfs_fsync_range(file, pos, pos + count - 1,
 +(file-f_flags  __O_SYNC) ? 0 : 1);
 + }
   goto out_unlock;
 
 out_trans_fail:
 diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
 index 643faa4..4f3d664 100644
 --- a/fs/ntfs/file.c
 +++ b/fs/ntfs/file.c
 @@ -2127,12 +2127,8 @@ static ssize_t ntfs_file_aio_write(struct kiocb *iocb, 
 const struct iovec *iov,
   mutex_lock(inode-i_mutex);
   ret = ntfs_file_aio_write_nolock(iocb, iov, nr_segs, iocb-ki_pos);
   mutex_unlock(inode-i_mutex);
 - if (ret  0) {
 - int err = generic_write_sync(file, iocb-ki_pos - ret, ret);
 - if (err  0)
 - ret = err;
 - }
 - return ret;
 +
 + return generic_write_sync(iocb, ret);
 }
 
 /**
 diff --git a/fs/udf/file.c b/fs/udf/file.c
 index bb15771..1cdabd0 100644
 --- a/fs/udf/file.c
 +++ b/fs/udf/file.c
 @@ -155,16 +155,9 @@ static ssize_t udf_file_write_iter(struct kiocb *iocb, 
 struct iov_iter *from)
   retval = __generic_file_write_iter(iocb, from);
   mutex_unlock(inode-i_mutex);
 
 - if (retval  0) {
 - ssize_t err;
 -
 + if (retval  0)
   mark_inode_dirty(inode);
 - err = generic_write_sync(file, iocb-ki_pos - retval, retval);
 - if (err  0)
 - retval = err;
 - }
 -
 - return retval;
 + return generic_write_sync(iocb, retval);
 }
 
 long udf_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
 index 0655915..a8cab66 100644
 --- a/fs/xfs/xfs_file.c
 +++ b/fs/xfs/xfs_file.c
 @@ -792,14 +792,8 @@ xfs_file_write_iter(
   ret = xfs_file_buffered_aio_write(iocb, from);
 
   if (ret  0) {
 - ssize_t err;
 -
   XFS_STATS_ADD(xs_write_bytes, ret);
 -
 - /* Handle various SYNC-type writes */
 - err = generic_write_sync(file, iocb-ki_pos - ret, ret);
 - if (err  0)
 - ret = err;
 + ret = generic_write_sync(iocb, ret);
   }
   return ret;
 }
 diff --git a/include/linux/fs.h b/include/linux/fs.h
 index eaebd99..7d0e116 100644
 --- a/include/linux/fs.h
 +++ b/include/linux/fs.h
 @@ -2242,13 +2242,7 @@ extern int filemap_fdatawrite_range(struct 
 address_space *mapping,
 extern int vfs_fsync_range(struct file *file, loff_t start, loff_t end,
  int datasync);
 extern int vfs_fsync(struct file *file, int datasync);
 -static inline int generic_write_sync(struct file *file, loff_t pos, loff_t 
 count)
 -{
 - if (!(file-f_flags  O_DSYNC)  !IS_SYNC(file-f_mapping-host))
 - return 0;
 - return vfs_fsync_range(file, pos, pos + count - 1,
 -(file-f_flags  __O_SYNC) ? 0 : 1);
 -}
 +extern int generic_write_sync(struct kiocb *iocb, loff_t count);
 extern void emergency_sync(void);
 extern void emergency_remount(void);
 #ifdef CONFIG_BLOCK
 diff --git a/mm/filemap.c b/mm/filemap.c
 index 09d3af3..6107058 100644
 --- a/mm/filemap.c
 +++ b/mm/filemap.c
 @@ -2664,6 +2664,24 @@ out:
 }
 EXPORT_SYMBOL(__generic_file_write_iter);
 
 +int generic_write_sync(struct kiocb *iocb, loff_t count)
 +{
 + struct file *file = iocb-ki_filp;
 +
 + if (count  0 
 + ((file-f_flags  O_DSYNC) || IS_SYNC(file-f_mapping-host))) {
 + bool fdatasync = !(file-f_flags  __O_SYNC);
 + ssize_t ret = 0;

That = 0 is pointless.  ret is overwritten unconditionally on the following 
line...

Other than that the NTFS bits are:

Acked-by: Anton Altaparmakov an...@tuxera.com

Best regards,

Anton

 +
 + ret = vfs_fsync_range(file, iocb-ki_pos - count,
 + iocb-ki_pos - 1, fdatasync);
 + if (ret  0)
 + return ret;
 + }
 + return count;
 +}
 +EXPORT_SYMBOL(generic_write_sync);
 +
 /**
  * generic_file_write_iter - write data to a file
  * @iocb: IO state structure
 @@ -2675,22 +2693,14 @@ EXPORT_SYMBOL(__generic_file_write_iter);
  */
 ssize_t generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 - struct file *file = iocb-ki_filp;
 - struct inode *inode = file-f_mapping-host;
 + struct inode *inode = iocb-ki_filp-f_mapping-host;
   ssize_t ret;
 
   mutex_lock(inode-i_mutex);
   ret = __generic_file_write_iter(iocb, from);
   mutex_unlock(inode-i_mutex);
 
 - if (ret  0) {
 - ssize_t err;
 -
 - err = generic_write_sync(file, iocb-ki_pos - ret, ret);
 - if (err  0)
 - ret = err;
 - }
 - return ret;
 + return generic_write_sync(iocb, ret);
 }
 EXPORT_SYMBOL(generic_file_write_iter);

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information

[git pull] Small NTFS update

2014-10-16 Thread Anton Altaparmakov

Hi Linus,

Here is a small NTFS update notably implementing FIBMAP ioctl for NTFS by 
adding the bmap address space operation.  People seem to still want 
FIBMAP.

Please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/aia21/ntfs.git master

to get these changes:

 Documentation/filesystems/ntfs.txt | 268 
-
 fs/ntfs/Makefile   |   2 +-
 fs/ntfs/aops.c | 163 --
 fs/ntfs/inode.c|  19 +--
 fs/ntfs/ntfs.h |   8 +-
 5 files changed, 167 insertions(+), 293 deletions(-)

Shortlog:

Anton Altaparmakov (4):
  NTFS: Split ntfs_aops into ntfs_normal_aops and ntfs_compressed_aop
in preparation for them diverging.
  NTFS: Remove changelog from Documentation/filesystems/ntfs.txt.
  NTFS: Add bmap address space operation needed for FIBMAP ioctl.
  NTFS: Bump version to 2.1.31.

Thanks a lot in advance!

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Senior Kernel Engineer, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[git pull] Small NTFS update

2014-10-16 Thread Anton Altaparmakov

Hi Linus,

Here is a small NTFS update notably implementing FIBMAP ioctl for NTFS by 
adding the bmap address space operation.  People seem to still want 
FIBMAP.

Please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/aia21/ntfs.git master

to get these changes:

 Documentation/filesystems/ntfs.txt | 268 
-
 fs/ntfs/Makefile   |   2 +-
 fs/ntfs/aops.c | 163 --
 fs/ntfs/inode.c|  19 +--
 fs/ntfs/ntfs.h |   8 +-
 5 files changed, 167 insertions(+), 293 deletions(-)

Shortlog:

Anton Altaparmakov (4):
  NTFS: Split ntfs_aops into ntfs_normal_aops and ntfs_compressed_aop
in preparation for them diverging.
  NTFS: Remove changelog from Documentation/filesystems/ntfs.txt.
  NTFS: Add bmap address space operation needed for FIBMAP ioctl.
  NTFS: Bump version to 2.1.31.

Thanks a lot in advance!

Best regards,

Anton
-- 
Anton Altaparmakov anton at tuxera.com (replace at with @)
Senior Kernel Engineer, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: WTF is d_add_ci() doing with negative dentries?

2014-10-13 Thread Anton Altaparmakov

Hi Al,

On 13 Oct 2014, at 03:14, Al Viro  wrote:
> On Mon, Oct 13, 2014 at 12:56:11AM +0100, Anton Altaparmakov wrote:
>> I am just wondering whether there might be error conditions in which we 
>> might end up with a (perhaps invalid) negative dentry in memory which could 
>> be found here?  Probably not a problem especially now that d_invalidate() 
>> cannot fail any more.
> 
> Huh?  Failing d_invalidate() on _negative_ dentry is flat-out impossible;
> it would be dropped just fine, and we wouldn't have found it in the first
> place.  Check what it used to do all way back to 2.2.0:
>if (dentry->d_count) {
>if (dentry->d_inode && S_ISDIR(dentry->d_inode->i_mode))
>return -EBUSY;
>}
> 
> So unless you care about 2.1.something (2.0 didn't have dcache at all),
> this scenario isn't possible.

That's fine then.  And no I don't care about 2.1.  I start caring at 2.6.15 at 
the moment.

> In any case, d_add_ci() users that might have negative dentries become
> positive cannot afford hashed negative dentries at all - at the very
> least they need to treat them as invalid in ->d_revalidate() in such
> situations.

Yes, that is what I do.  I have my own d_revalidate method which reports all 
negative dentries as invalid any time a new name is added in the parent 
directory.  In fact I set d_time of each dentry to i_version of the parent 
directory inode and on each "add name to directory" I bump i_version of the 
parent directory and d_revalidate checks d_time against 
d_parent->d_inode->i_version and if not equal the negative dentry is considered 
invalid and that seems to work fine (I ignore d_time on positive dentries in 
d_revalidate).

> Exactly because having a hashed valid negative dentry for
> FuBaR after e.g.  mkdir fubar will really hurt - mkdir won't have any way
> to know that old dentry was there; there was no variant of fubar in directory
> prior to that mkdir (FuBaR _was_ negative) and there's nothing to suggest
> looking for it.  So it won't be noticed and it'll bloody well stay negative
> and hashed.  I.e. stat FuBaR; mkdir fubar; stat FuBaR will have the second
> stat find dentry still hashed and valid negative.
> 
> You can get away with that if you store something like timestamp[1] of
> the parent directory in those negative dentries and check that in
> ->d_revalidate().  But that will work just fine, since d_add_ci() is
> serialized by ->i_mutex held on parent and whatever it was that added your
> "exact spelling" into directory has made all preexisting negative dentries
> invalid...

Yes, that is basically what I do except I use i_version on the parent rather 
than its d_time and I don't set d_time to zero (I always set it to i_version of 
parent instead though I doubt it matters in the slightest - I could equally not 
be setting it at all for positive dentries given I never check it for them).

Best regards,

Anton

> [1] for arbitrary values of time - e.g.
>   on positive lookup set ->d_time to 0
>   on negative lookup set ->d_time to that of parent dentry
>   on mkdir set ->d_time to 0
>   on unlink, rmdir and rename victim copy ->d_time from parent dentry
>   on any directory modification bump its ->d_time
>   on d_revalidate of negative dentry compare ->d_time with that of parent
>   dentry and declare invalid on mismatch
> will do just fine.

-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: WTF is d_add_ci() doing with negative dentries?

2014-10-13 Thread Anton Altaparmakov

Hi Al,

On 13 Oct 2014, at 03:14, Al Viro v...@zeniv.linux.org.uk wrote:
 On Mon, Oct 13, 2014 at 12:56:11AM +0100, Anton Altaparmakov wrote:
 I am just wondering whether there might be error conditions in which we 
 might end up with a (perhaps invalid) negative dentry in memory which could 
 be found here?  Probably not a problem especially now that d_invalidate() 
 cannot fail any more.
 
 Huh?  Failing d_invalidate() on _negative_ dentry is flat-out impossible;
 it would be dropped just fine, and we wouldn't have found it in the first
 place.  Check what it used to do all way back to 2.2.0:
if (dentry-d_count) {
if (dentry-d_inode  S_ISDIR(dentry-d_inode-i_mode))
return -EBUSY;
}
 
 So unless you care about 2.1.something (2.0 didn't have dcache at all),
 this scenario isn't possible.

That's fine then.  And no I don't care about 2.1.  I start caring at 2.6.15 at 
the moment.

 In any case, d_add_ci() users that might have negative dentries become
 positive cannot afford hashed negative dentries at all - at the very
 least they need to treat them as invalid in -d_revalidate() in such
 situations.

Yes, that is what I do.  I have my own d_revalidate method which reports all 
negative dentries as invalid any time a new name is added in the parent 
directory.  In fact I set d_time of each dentry to i_version of the parent 
directory inode and on each add name to directory I bump i_version of the 
parent directory and d_revalidate checks d_time against 
d_parent-d_inode-i_version and if not equal the negative dentry is considered 
invalid and that seems to work fine (I ignore d_time on positive dentries in 
d_revalidate).

 Exactly because having a hashed valid negative dentry for
 FuBaR after e.g.  mkdir fubar will really hurt - mkdir won't have any way
 to know that old dentry was there; there was no variant of fubar in directory
 prior to that mkdir (FuBaR _was_ negative) and there's nothing to suggest
 looking for it.  So it won't be noticed and it'll bloody well stay negative
 and hashed.  I.e. stat FuBaR; mkdir fubar; stat FuBaR will have the second
 stat find dentry still hashed and valid negative.
 
 You can get away with that if you store something like timestamp[1] of
 the parent directory in those negative dentries and check that in
 -d_revalidate().  But that will work just fine, since d_add_ci() is
 serialized by -i_mutex held on parent and whatever it was that added your
 exact spelling into directory has made all preexisting negative dentries
 invalid...

Yes, that is basically what I do except I use i_version on the parent rather 
than its d_time and I don't set d_time to zero (I always set it to i_version of 
parent instead though I doubt it matters in the slightest - I could equally not 
be setting it at all for positive dentries given I never check it for them).

Best regards,

Anton

 [1] for arbitrary values of time - e.g.
   on positive lookup set -d_time to 0
   on negative lookup set -d_time to that of parent dentry
   on mkdir set -d_time to 0
   on unlink, rmdir and rename victim copy -d_time from parent dentry
   on any directory modification bump its -d_time
   on d_revalidate of negative dentry compare -d_time with that of parent
   dentry and declare invalid on mismatch
 will do just fine.

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: WTF is d_add_ci() doing with negative dentries?

2014-10-12 Thread Anton Altaparmakov

Hi Al,

On 12 Oct 2014, at 23:18, Al Viro  wrote:
>   AFAICS, if d_add_ci() ever finds a negative hashed dentry for
> exact name, it's already buggered.  Because right *before* that
> d_add_ci() lookup for exact name would've turned valid negative.

Christoph copied d_add_ci() from code I wrote for NTFS so you can blame me for 
it.  (-;

Do you mean that given the exact name exists on disk, there cannot be a 
negative dentry for it in memory, i.e. there would either be no dentry in 
memory or it would be a positive dentry in memory?

If so then that makes sense, yes.

I am just wondering whether there might be error conditions in which we might 
end up with a (perhaps invalid) negative dentry in memory which could be found 
here?  Probably not a problem especially now that d_invalidate() cannot fail 
any more.

Is it worth adding a BUG_ON(!found->d_inode); to ensure sanity/catch bugs?

> IOW, the whole thing ought to be
>found = d_hash_and_lookup(dentry->d_parent, name);
>   if (found) {
>   iput(inode);
>   return found;
>   }
>   new = d_alloc(dentry->d_parent, name);
>   if (!new) {
>   iput(inode);
>   return ERR_PTR(-ENOMEM);
>   }
>   found = d_splice_alias(inode, new);
>   if (found) {
>   dput(new);
>   return found;
>   }
>   return new;
> Moreover, it might very well be better to pass dentry->d_parent instead
> of dentry...  Objections?

Yes, that bit makes perfect sense given we only ever use dentry->d_parent.

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: WTF is d_add_ci() doing with negative dentries?

2014-10-12 Thread Anton Altaparmakov

Hi Al,

On 12 Oct 2014, at 23:18, Al Viro v...@zeniv.linux.org.uk wrote:
   AFAICS, if d_add_ci() ever finds a negative hashed dentry for
 exact name, it's already buggered.  Because right *before* that
 d_add_ci() lookup for exact name would've turned valid negative.

Christoph copied d_add_ci() from code I wrote for NTFS so you can blame me for 
it.  (-;

Do you mean that given the exact name exists on disk, there cannot be a 
negative dentry for it in memory, i.e. there would either be no dentry in 
memory or it would be a positive dentry in memory?

If so then that makes sense, yes.

I am just wondering whether there might be error conditions in which we might 
end up with a (perhaps invalid) negative dentry in memory which could be found 
here?  Probably not a problem especially now that d_invalidate() cannot fail 
any more.

Is it worth adding a BUG_ON(!found-d_inode); to ensure sanity/catch bugs?

 IOW, the whole thing ought to be
found = d_hash_and_lookup(dentry-d_parent, name);
   if (found) {
   iput(inode);
   return found;
   }
   new = d_alloc(dentry-d_parent, name);
   if (!new) {
   iput(inode);
   return ERR_PTR(-ENOMEM);
   }
   found = d_splice_alias(inode, new);
   if (found) {
   dput(new);
   return found;
   }
   return new;
 Moreover, it might very well be better to pass dentry-d_parent instead
 of dentry...  Objections?

Yes, that bit makes perfect sense given we only ever use dentry-d_parent.

Best regards,

Anton
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH] NTFS: Remove bogus space.

Hi Andrew,

Forgot to say that this patch is from Andrea Gilmini originally (I had to re-do 
it as it is an old patch and the line numbers had changed)...

Best regards,

Anton

On 3 Oct 2014, at 00:44, Anton Altaparmakov  wrote:

> fs/ntfs/debug.c:124: WARNING: space prohibited between function name and 
> open parenthesis '('
> 
> Signed-off-by: Andrea Gelmini 
> Signed-off-by: Anton Altaparmakov 
> ---
> Hi Andrew,
> 
> Can you please take this simple patch and push it to Linus at some point?
> 
> Thanks a lot in advance!
> 
> Best regards,
> 
>   Anton
> -- 
> Anton Altaparmakov  (replace at with @)
> Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
> Linux NTFS maintainer, http://www.linux-ntfs.org/
> 
> diff --git a/fs/ntfs/debug.c b/fs/ntfs/debug.c
> index dd6103c..825a54e 100644
> --- a/fs/ntfs/debug.c
> +++ b/fs/ntfs/debug.c
> @@ -112,7 +112,7 @@ void __ntfs_error(const char *function, const struct 
> super_block *sb,
> /* If 1, output debug messages, and if 0, don't. */
> int debug_msgs = 0;
> 
> -void __ntfs_debug (const char *file, int line, const char *function,
> +void __ntfs_debug(const char *file, int line, const char *function,
>   const char *fmt, ...)
> {
>   struct va_format vaf;
-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] NTFS: Remove bogus space.

fs/ntfs/debug.c:124: WARNING: space prohibited between function name and 
open parenthesis '('

Signed-off-by: Andrea Gelmini 
Signed-off-by: Anton Altaparmakov 
---
Hi Andrew,

Can you please take this simple patch and push it to Linus at some point?

Thanks a lot in advance!

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/

diff --git a/fs/ntfs/debug.c b/fs/ntfs/debug.c
index dd6103c..825a54e 100644
--- a/fs/ntfs/debug.c
+++ b/fs/ntfs/debug.c
@@ -112,7 +112,7 @@ void __ntfs_error(const char *function, const struct 
super_block *sb,
 /* If 1, output debug messages, and if 0, don't. */
 int debug_msgs = 0;
 
-void __ntfs_debug (const char *file, int line, const char *function,
+void __ntfs_debug(const char *file, int line, const char *function,
const char *fmt, ...)
 {
struct va_format vaf;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] NTFS: Remove bogus space.

fs/ntfs/debug.c:124: WARNING: space prohibited between function name and 
open parenthesis '('

Signed-off-by: Andrea Gelmini andrea.gelm...@gelma.net
Signed-off-by: Anton Altaparmakov an...@tuxera.com
---
Hi Andrew,

Can you please take this simple patch and push it to Linus at some point?

Thanks a lot in advance!

Best regards,

Anton
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/

diff --git a/fs/ntfs/debug.c b/fs/ntfs/debug.c
index dd6103c..825a54e 100644
--- a/fs/ntfs/debug.c
+++ b/fs/ntfs/debug.c
@@ -112,7 +112,7 @@ void __ntfs_error(const char *function, const struct 
super_block *sb,
 /* If 1, output debug messages, and if 0, don't. */
 int debug_msgs = 0;
 
-void __ntfs_debug (const char *file, int line, const char *function,
+void __ntfs_debug(const char *file, int line, const char *function,
const char *fmt, ...)
 {
struct va_format vaf;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] NTFS: Remove bogus space.

Hi Andrew,

Forgot to say that this patch is from Andrea Gilmini originally (I had to re-do 
it as it is an old patch and the line numbers had changed)...

Best regards,

Anton

On 3 Oct 2014, at 00:44, Anton Altaparmakov ai...@cam.ac.uk wrote:

 fs/ntfs/debug.c:124: WARNING: space prohibited between function name and 
 open parenthesis '('
 
 Signed-off-by: Andrea Gelmini andrea.gelm...@gelma.net
 Signed-off-by: Anton Altaparmakov an...@tuxera.com
 ---
 Hi Andrew,
 
 Can you please take this simple patch and push it to Linus at some point?
 
 Thanks a lot in advance!
 
 Best regards,
 
   Anton
 -- 
 Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
 Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
 Linux NTFS maintainer, http://www.linux-ntfs.org/
 
 diff --git a/fs/ntfs/debug.c b/fs/ntfs/debug.c
 index dd6103c..825a54e 100644
 --- a/fs/ntfs/debug.c
 +++ b/fs/ntfs/debug.c
 @@ -112,7 +112,7 @@ void __ntfs_error(const char *function, const struct 
 super_block *sb,
 /* If 1, output debug messages, and if 0, don't. */
 int debug_msgs = 0;
 
 -void __ntfs_debug (const char *file, int line, const char *function,
 +void __ntfs_debug(const char *file, int line, const char *function,
   const char *fmt, ...)
 {
   struct va_format vaf;
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH] Fix nasty 32-bit overflow bug in buffer i/o code.

Hi Linus,

On 22 Sep 2014, at 16:33, Linus Torvalds  wrote:
> On Mon, Sep 22, 2014 at 8:29 AM, Anton Altaparmakov  wrote:
>> 
>> You could do "block & ~(sector_t)(size - 1)" instead of "(sector_t)index << 
>> sizebits" if you prefer but not sure that is an improvement!
> 
> No, it would be even worse. Something like
> 
>  block & ~(sector_t)((size >> 9) - 1)
> 
> because block is the sector number (ie 512-byte) and size is in bytes.

Oops, sorry.  But I think you got it wrong, too as you are ignoring the 
PAGE_SIZE - as was I but it is what we need to align to in addition to the 
problem of "size" being in bytes.  So I think the correct mask is actually 
based on sizebits which reflects the number of blocks per page thus:

block & ~(sector_t)((1 << sizebits) - 1)

In any case the shift is the lesser evil I think as it is at least obviously 
correct whilst getting the right mask has taken us a few iterations of 
correcting each other! (-:

PS. Thank you for taking my patch and correcting the misleading description!

Best regards,

Anton

>   Linus

-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Fix nasty 32-bit overflow bug in buffer i/o code.

Hi Linus,

On 22 Sep 2014, at 16:18, Linus Torvalds  wrote:
> On Sun, Sep 21, 2014 at 5:53 PM, Anton Altaparmakov  wrote:
>> 
>> This patch fixes this issue by type casting "index" to sector_t before
>> doing the left shift.
> 
> Ugh. Does the simpler patch to just pass in "block" work as well?

That doesn't work because nothing rounds down "block" to the first block in the 
page and init_page_buffers() requires "block" to be the first block in the page.

The shift right followed by shift left achieves the "rounding down" required.

You could do "block & ~(sector_t)(size - 1)" instead of "(sector_t)index << 
sizebits" if you prefer but not sure that is an improvement!

Best regards,

Anton

>Linus
> 

-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Fix nasty 32-bit overflow bug in buffer i/o code.

Hi,

On 22 Sep 2014, at 11:36, Hugh Dickins  wrote:
> On Mon, 22 Sep 2014, Anton Altaparmakov wrote:
>> On 22 Sep 2014, at 05:43, Hugh Dickins  wrote:
>>> On Mon, 22 Sep 2014, Anton Altaparmakov wrote:
>>>> Any code that uses __getblk() and thus bread(), breadahead(), sb_bread(), 
>>>> sb_breadahead(), sb_getblk(), and calls it using a 64-bit block on a 
>>>> 32-bit arch (where "long" is 32-bit) causes an inifinite loop in 
>>>> __getblk_slow() with an infinite stream of errors logged to dmesg like 
>>>> this:
>>>> 
>>>> __find_get_block_slow() failed. block=6740375944, b_blocknr=2445408648
>>>> b_state=0x0020, b_size=512
>>>> device sda1 blocksize: 512
>>>> 
>>>> Note how in hex block is 0x191C1F988 and b_blocknr is 0x91C1F988 i.e. the 
>>>> top 32-bits are missing (in this case the 0x1 at the top).
>>>> 
>>>> This is because grow_dev_page() was broken in commit 676ce6d5ca30: "block: 
>>>> replace __getblk_slow misfix by grow_dev_page fix" by Hugh Dickins so that 
>>>> it now has a 32-bit overflow due to shifting the block value to the right 
>>>> so it fits in 32-bits and storing the result in pgoff_t variable which is 
>>>> 32-bit and then passing the pgoff_t variable left-shifted as the block 
>>>> number which causes the top bits to get lost as the pgoff_t is not type 
>>>> cast to sector_t / 64-bit before the shift.
>>>> 
>>>> This patch fixes this issue by type casting "index" to sector_t before 
>>>> doing the left shift.
>>>> 
>>>> Note this is not a theoretical bug but has been seen in the field on a 
>>>> 4TiB hard drive with logical sector size 512 bytes.
>>>> 
>>>> This patch has been verified to fix the infinite loop problem on 3.17-rc5 
>>>> kernel using a 4TB disk image mounted using "-o loop".  Without this patch 
>>>> doing a "find /nt" where /nt is an NTFS volume causes the inifinite loop 
>>>> 100% reproducibly whilst with the patch it works fine as expected.
>>>> 
>>>> Signed-off-by: Anton Altaparmakov 
>>>> Cc: sta...@vger.kernel.org # 3.6 3.8 3.10 3.12 3.14 3.16
>>> 
>>> Acked-by: Hugh Dickins 
>>> 
>>> Yes indeed, that's bad, and entirely my fault (though it took me a while
>>> to see how the "block = index << sizebits" which was there before was okay,
>> 
>> Ouch.  I failed to see that (I admit I did not pay too much attention to the 
>> original code - I just used "git blame" to find out which commit put that 
>> code in place - my bad).  It was never ok!  I went back to 2.6.12-rc2 
>> (original commit to git Linus' kernel repository) and that is also affected. 
>>  That implies this is the first time anyone has used a 4TiB disk with 512 
>> byte sectors with NTFS where Windows had previously created a file/directory 
>> with an attribute list attribute in it.  -  That is the only metadata type 
>> we use sb_bread() for and the disk image from the customer does contain it 
>> and I verified that is where the inifinite loop comes from.
> 
> Ow, that line needs some truncating itself :)
> 
> You generously interpret my words as seeing that for myself.
> No, thank you for following up: I had persuaded myself when writing
> before, that index would be promoted from pgoff_t to sector_t before
> shifting in the original assignment, but not when I passed it as arg.
> 
> I've resorted to writing a proggy to check, and it looks like I was
> earlier confused, and you are right, and that code was "always" wrong.
> 
> Not much use of 4TiB disks on 32-bit kernels I suppose.

Actually on embedded devices like broadband routers/home network NAS boxes/PVR 
boxes, etc it is very common to have 32-bit (on ARM/MIPS or more recently also 
x86) and more and more people are plugging in larger disks to store their 
music/videos/movies, etc...  So this is something that is on the up and going 
to become an increasing problem as large disks get cheaper and cheaper.

>> So it appears sb_bread() and friends have always been broken on 32-bit 
>> architectures when accessing blocks outside the range 2^32 - 1 and it 
>> appears google finds lots of occurrences of such infinite loops being 
>> reported but the fixes have always been to not make the calls in the first 
>> place.  No-one seems to have recognized that there is a genuine problem here 
>> before.
>> 
>> Still my patch stands correct and should be applied to

Re: [PATCH] Fix nasty 32-bit overflow bug in buffer i/o code.

Hi Hugh,

On 22 Sep 2014, at 05:43, Hugh Dickins  wrote:
> On Mon, 22 Sep 2014, Anton Altaparmakov wrote:
>> Any code that uses __getblk() and thus bread(), breadahead(), sb_bread(), 
>> sb_breadahead(), sb_getblk(), and calls it using a 64-bit block on a 
>> 32-bit arch (where "long" is 32-bit) causes an inifinite loop in 
>> __getblk_slow() with an infinite stream of errors logged to dmesg like 
>> this:
>> 
>> __find_get_block_slow() failed. block=6740375944, b_blocknr=2445408648
>> b_state=0x0020, b_size=512
>> device sda1 blocksize: 512
>> 
>> Note how in hex block is 0x191C1F988 and b_blocknr is 0x91C1F988 i.e. the 
>> top 32-bits are missing (in this case the 0x1 at the top).
>> 
>> This is because grow_dev_page() was broken in commit 676ce6d5ca30: "block: 
>> replace __getblk_slow misfix by grow_dev_page fix" by Hugh Dickins so that 
>> it now has a 32-bit overflow due to shifting the block value to the right 
>> so it fits in 32-bits and storing the result in pgoff_t variable which is 
>> 32-bit and then passing the pgoff_t variable left-shifted as the block 
>> number which causes the top bits to get lost as the pgoff_t is not type 
>> cast to sector_t / 64-bit before the shift.
>> 
>> This patch fixes this issue by type casting "index" to sector_t before 
>> doing the left shift.
>> 
>> Note this is not a theoretical bug but has been seen in the field on a 
>> 4TiB hard drive with logical sector size 512 bytes.
>> 
>> This patch has been verified to fix the infinite loop problem on 3.17-rc5 
>> kernel using a 4TB disk image mounted using "-o loop".  Without this patch 
>> doing a "find /nt" where /nt is an NTFS volume causes the inifinite loop 
>> 100% reproducibly whilst with the patch it works fine as expected.
>> 
>> Signed-off-by: Anton Altaparmakov 
>> Cc: sta...@vger.kernel.org # 3.6 3.8 3.10 3.12 3.14 3.16
> 
> Acked-by: Hugh Dickins 
> 
> Yes indeed, that's bad, and entirely my fault (though it took me a while
> to see how the "block = index << sizebits" which was there before was okay,

Ouch.  I failed to see that (I admit I did not pay too much attention to the 
original code - I just used "git blame" to find out which commit put that code 
in place - my bad).  It was never ok!  I went back to 2.6.12-rc2 (original 
commit to git Linus' kernel repository) and that is also affected.  That 
implies this is the first time anyone has used a 4TiB disk with 512 byte 
sectors with NTFS where Windows had previously created a file/directory with an 
attribute list attribute in it.  -  That is the only metadata type we use 
sb_bread() for and the disk image from the customer does contain it and I 
verified that is where the inifinite loop comes from.

So it appears sb_bread() and friends have always been broken on 32-bit 
architectures when accessing blocks outside the range 2^32 - 1 and it appears 
google finds lots of occurrences of such infinite loops being reported but the 
fixes have always been to not make the calls in the first place.  No-one seems 
to have recognized that there is a genuine problem here before.

Still my patch stands correct and should be applied to all kernel versions that 
have your patch and older kernels should have an equivalent patch applied to 
fix them, too for people who run very old kernels.

> but my passing "index << sizebits" as arg to function very much not okay).
> Thank you, Anton.

You are welcome.

> But I see my commit was marked for stable 3.0 3.2 3.4 3.5: so your fix
> is needed in 3.2 and 3.4 longterm also (the others now beyond life).

You are quite right.  It needs to go back to all those kernels, too.  Thank you!

Best regards,

Anton

> Hugh
> 
>> ---
>> 
>> Linus, can you please apply this?  Alternatively, Andrew, can you please 
>> pick this up and send it to Linus?
>> 
>> It would be good it it can be applied for 3.17 as well as to all stable 
>> kernels >= 3.6 as they are all broken!
>> 
>> Thanks a lot in advance!
>> 
>> Best regards,
>> 
>>  Anton
>> -- 
>> Anton Altaparmakov  (replace at with @)
>> Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
>> Linux NTFS maintainer, http://www.linux-ntfs.org/
>> 
>> diff --git a/fs/buffer.c b/fs/buffer.c
>> index 8f05111..3588a80 100644
>> --- a/fs/buffer.c
>> +++ b/fs/buffer.c
>> @@ -1022,7 +1022,8 @@ grow_dev_page(struct block_device *bdev, sector_t 
>> block,
>>  bh = page_buffers(page);
>>  if (bh->b_size == size) {
>>

Re: [PATCH] Fix nasty 32-bit overflow bug in buffer i/o code.

Hi Hugh,

On 22 Sep 2014, at 05:43, Hugh Dickins hu...@google.com wrote:
 On Mon, 22 Sep 2014, Anton Altaparmakov wrote:
 Any code that uses __getblk() and thus bread(), breadahead(), sb_bread(), 
 sb_breadahead(), sb_getblk(), and calls it using a 64-bit block on a 
 32-bit arch (where long is 32-bit) causes an inifinite loop in 
 __getblk_slow() with an infinite stream of errors logged to dmesg like 
 this:
 
 __find_get_block_slow() failed. block=6740375944, b_blocknr=2445408648
 b_state=0x0020, b_size=512
 device sda1 blocksize: 512
 
 Note how in hex block is 0x191C1F988 and b_blocknr is 0x91C1F988 i.e. the 
 top 32-bits are missing (in this case the 0x1 at the top).
 
 This is because grow_dev_page() was broken in commit 676ce6d5ca30: block: 
 replace __getblk_slow misfix by grow_dev_page fix by Hugh Dickins so that 
 it now has a 32-bit overflow due to shifting the block value to the right 
 so it fits in 32-bits and storing the result in pgoff_t variable which is 
 32-bit and then passing the pgoff_t variable left-shifted as the block 
 number which causes the top bits to get lost as the pgoff_t is not type 
 cast to sector_t / 64-bit before the shift.
 
 This patch fixes this issue by type casting index to sector_t before 
 doing the left shift.
 
 Note this is not a theoretical bug but has been seen in the field on a 
 4TiB hard drive with logical sector size 512 bytes.
 
 This patch has been verified to fix the infinite loop problem on 3.17-rc5 
 kernel using a 4TB disk image mounted using -o loop.  Without this patch 
 doing a find /nt where /nt is an NTFS volume causes the inifinite loop 
 100% reproducibly whilst with the patch it works fine as expected.
 
 Signed-off-by: Anton Altaparmakov ai...@cantab.net
 Cc: sta...@vger.kernel.org # 3.6 3.8 3.10 3.12 3.14 3.16
 
 Acked-by: Hugh Dickins hu...@google.com
 
 Yes indeed, that's bad, and entirely my fault (though it took me a while
 to see how the block = index  sizebits which was there before was okay,

Ouch.  I failed to see that (I admit I did not pay too much attention to the 
original code - I just used git blame to find out which commit put that code 
in place - my bad).  It was never ok!  I went back to 2.6.12-rc2 (original 
commit to git Linus' kernel repository) and that is also affected.  That 
implies this is the first time anyone has used a 4TiB disk with 512 byte 
sectors with NTFS where Windows had previously created a file/directory with an 
attribute list attribute in it.  -  That is the only metadata type we use 
sb_bread() for and the disk image from the customer does contain it and I 
verified that is where the inifinite loop comes from.

So it appears sb_bread() and friends have always been broken on 32-bit 
architectures when accessing blocks outside the range 2^32 - 1 and it appears 
google finds lots of occurrences of such infinite loops being reported but the 
fixes have always been to not make the calls in the first place.  No-one seems 
to have recognized that there is a genuine problem here before.

Still my patch stands correct and should be applied to all kernel versions that 
have your patch and older kernels should have an equivalent patch applied to 
fix them, too for people who run very old kernels.

 but my passing index  sizebits as arg to function very much not okay).
 Thank you, Anton.

You are welcome.

 But I see my commit was marked for stable 3.0 3.2 3.4 3.5: so your fix
 is needed in 3.2 and 3.4 longterm also (the others now beyond life).

You are quite right.  It needs to go back to all those kernels, too.  Thank you!

Best regards,

Anton

 Hugh
 
 ---
 
 Linus, can you please apply this?  Alternatively, Andrew, can you please 
 pick this up and send it to Linus?
 
 It would be good it it can be applied for 3.17 as well as to all stable 
 kernels = 3.6 as they are all broken!
 
 Thanks a lot in advance!
 
 Best regards,
 
  Anton
 -- 
 Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
 Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
 Linux NTFS maintainer, http://www.linux-ntfs.org/
 
 diff --git a/fs/buffer.c b/fs/buffer.c
 index 8f05111..3588a80 100644
 --- a/fs/buffer.c
 +++ b/fs/buffer.c
 @@ -1022,7 +1022,8 @@ grow_dev_page(struct block_device *bdev, sector_t 
 block,
  bh = page_buffers(page);
  if (bh-b_size == size) {
  end_block = init_page_buffers(page, bdev,
 -index  sizebits, size);
 +(sector_t)index  sizebits,
 +size);
  goto done;
  }
  if (!try_to_free_buffers(page))
 @@ -1043,7 +1044,8 @@ grow_dev_page(struct block_device *bdev, sector_t 
 block,
   */
  spin_lock(inode-i_mapping-private_lock);
  link_dev_buffers(page, bh);
 -end_block = init_page_buffers(page, bdev, index  sizebits, size);
 +end_block

Re: [PATCH] Fix nasty 32-bit overflow bug in buffer i/o code.

Hi,

On 22 Sep 2014, at 11:36, Hugh Dickins hu...@google.com wrote:
 On Mon, 22 Sep 2014, Anton Altaparmakov wrote:
 On 22 Sep 2014, at 05:43, Hugh Dickins hu...@google.com wrote:
 On Mon, 22 Sep 2014, Anton Altaparmakov wrote:
 Any code that uses __getblk() and thus bread(), breadahead(), sb_bread(), 
 sb_breadahead(), sb_getblk(), and calls it using a 64-bit block on a 
 32-bit arch (where long is 32-bit) causes an inifinite loop in 
 __getblk_slow() with an infinite stream of errors logged to dmesg like 
 this:
 
 __find_get_block_slow() failed. block=6740375944, b_blocknr=2445408648
 b_state=0x0020, b_size=512
 device sda1 blocksize: 512
 
 Note how in hex block is 0x191C1F988 and b_blocknr is 0x91C1F988 i.e. the 
 top 32-bits are missing (in this case the 0x1 at the top).
 
 This is because grow_dev_page() was broken in commit 676ce6d5ca30: block: 
 replace __getblk_slow misfix by grow_dev_page fix by Hugh Dickins so that 
 it now has a 32-bit overflow due to shifting the block value to the right 
 so it fits in 32-bits and storing the result in pgoff_t variable which is 
 32-bit and then passing the pgoff_t variable left-shifted as the block 
 number which causes the top bits to get lost as the pgoff_t is not type 
 cast to sector_t / 64-bit before the shift.
 
 This patch fixes this issue by type casting index to sector_t before 
 doing the left shift.
 
 Note this is not a theoretical bug but has been seen in the field on a 
 4TiB hard drive with logical sector size 512 bytes.
 
 This patch has been verified to fix the infinite loop problem on 3.17-rc5 
 kernel using a 4TB disk image mounted using -o loop.  Without this patch 
 doing a find /nt where /nt is an NTFS volume causes the inifinite loop 
 100% reproducibly whilst with the patch it works fine as expected.
 
 Signed-off-by: Anton Altaparmakov ai...@cantab.net
 Cc: sta...@vger.kernel.org # 3.6 3.8 3.10 3.12 3.14 3.16
 
 Acked-by: Hugh Dickins hu...@google.com
 
 Yes indeed, that's bad, and entirely my fault (though it took me a while
 to see how the block = index  sizebits which was there before was okay,
 
 Ouch.  I failed to see that (I admit I did not pay too much attention to the 
 original code - I just used git blame to find out which commit put that 
 code in place - my bad).  It was never ok!  I went back to 2.6.12-rc2 
 (original commit to git Linus' kernel repository) and that is also affected. 
  That implies this is the first time anyone has used a 4TiB disk with 512 
 byte sectors with NTFS where Windows had previously created a file/directory 
 with an attribute list attribute in it.  -  That is the only metadata type 
 we use sb_bread() for and the disk image from the customer does contain it 
 and I verified that is where the inifinite loop comes from.
 
 Ow, that line needs some truncating itself :)
 
 You generously interpret my words as seeing that for myself.
 No, thank you for following up: I had persuaded myself when writing
 before, that index would be promoted from pgoff_t to sector_t before
 shifting in the original assignment, but not when I passed it as arg.
 
 I've resorted to writing a proggy to check, and it looks like I was
 earlier confused, and you are right, and that code was always wrong.
 
 Not much use of 4TiB disks on 32-bit kernels I suppose.

Actually on embedded devices like broadband routers/home network NAS boxes/PVR 
boxes, etc it is very common to have 32-bit (on ARM/MIPS or more recently also 
x86) and more and more people are plugging in larger disks to store their 
music/videos/movies, etc...  So this is something that is on the up and going 
to become an increasing problem as large disks get cheaper and cheaper.

 So it appears sb_bread() and friends have always been broken on 32-bit 
 architectures when accessing blocks outside the range 2^32 - 1 and it 
 appears google finds lots of occurrences of such infinite loops being 
 reported but the fixes have always been to not make the calls in the first 
 place.  No-one seems to have recognized that there is a genuine problem here 
 before.
 
 Still my patch stands correct and should be applied to all kernel versions 
 that have your patch and older kernels should have an equivalent patch 
 applied to fix them, too for people who run very old kernels.
 
 Yes, though I'm now uncertain whether your patch is a bugfix or an
 enhancement.  Definitely a bugfix, given the infinite loops.  But the
 oldest code (index = block  sizebits; block = index  sizebits;)
 is so clearly trying to truncate block, that I wonder what departing
 from that might be letting us in for.

Having looked at it, I think I understand the original code and intention - it 
is not truncating the top bits, it is truncating the bottom bits, i.e. it is 
rounding down so that block becomes the first block in the page as that is 
what is needed to be passed into grow_dev_page().  And the shift right followed 
by shift left makes those bottom bits fall off.

A perfectly valid

Re: [PATCH] Fix nasty 32-bit overflow bug in buffer i/o code.

Hi Linus,

On 22 Sep 2014, at 16:18, Linus Torvalds torva...@linux-foundation.org wrote:
 On Sun, Sep 21, 2014 at 5:53 PM, Anton Altaparmakov ai...@cam.ac.uk wrote:
 
 This patch fixes this issue by type casting index to sector_t before
 doing the left shift.
 
 Ugh. Does the simpler patch to just pass in block work as well?

That doesn't work because nothing rounds down block to the first block in the 
page and init_page_buffers() requires block to be the first block in the page.

The shift right followed by shift left achieves the rounding down required.

You could do block  ~(sector_t)(size - 1) instead of (sector_t)index  
sizebits if you prefer but not sure that is an improvement!

Best regards,

Anton

Linus
 patch.diff


-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH] Fix nasty 32-bit overflow bug in buffer i/o code.

Hi Linus,

On 22 Sep 2014, at 16:33, Linus Torvalds torva...@linux-foundation.org wrote:
 On Mon, Sep 22, 2014 at 8:29 AM, Anton Altaparmakov ai...@cam.ac.uk wrote:
 
 You could do block  ~(sector_t)(size - 1) instead of (sector_t)index  
 sizebits if you prefer but not sure that is an improvement!
 
 No, it would be even worse. Something like
 
  block  ~(sector_t)((size  9) - 1)
 
 because block is the sector number (ie 512-byte) and size is in bytes.

Oops, sorry.  But I think you got it wrong, too as you are ignoring the 
PAGE_SIZE - as was I but it is what we need to align to in addition to the 
problem of size being in bytes.  So I think the correct mask is actually 
based on sizebits which reflects the number of blocks per page thus:

block  ~(sector_t)((1  sizebits) - 1)

In any case the shift is the lesser evil I think as it is at least obviously 
correct whilst getting the right mask has taken us a few iterations of 
correcting each other! (-:

PS. Thank you for taking my patch and correcting the misleading description!

Best regards,

Anton

   Linus

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

[PATCH] Fix nasty 32-bit overflow bug in buffer i/o code.

2014-09-21 Thread Anton Altaparmakov

Any code that uses __getblk() and thus bread(), breadahead(), sb_bread(), 
sb_breadahead(), sb_getblk(), and calls it using a 64-bit block on a 
32-bit arch (where "long" is 32-bit) causes an inifinite loop in 
__getblk_slow() with an infinite stream of errors logged to dmesg like 
this:

__find_get_block_slow() failed. block=6740375944, b_blocknr=2445408648
b_state=0x0020, b_size=512
device sda1 blocksize: 512

Note how in hex block is 0x191C1F988 and b_blocknr is 0x91C1F988 i.e. the 
top 32-bits are missing (in this case the 0x1 at the top).

This is because grow_dev_page() was broken in commit 676ce6d5ca30: "block: 
replace __getblk_slow misfix by grow_dev_page fix" by Hugh Dickins so that 
it now has a 32-bit overflow due to shifting the block value to the right 
so it fits in 32-bits and storing the result in pgoff_t variable which is 
32-bit and then passing the pgoff_t variable left-shifted as the block 
number which causes the top bits to get lost as the pgoff_t is not type 
cast to sector_t / 64-bit before the shift.

This patch fixes this issue by type casting "index" to sector_t before 
doing the left shift.

Note this is not a theoretical bug but has been seen in the field on a 
4TiB hard drive with logical sector size 512 bytes.

This patch has been verified to fix the infinite loop problem on 3.17-rc5 
kernel using a 4TB disk image mounted using "-o loop".  Without this patch 
doing a "find /nt" where /nt is an NTFS volume causes the inifinite loop 
100% reproducibly whilst with the patch it works fine as expected.

Signed-off-by: Anton Altaparmakov 
Cc: sta...@vger.kernel.org # 3.6 3.8 3.10 3.12 3.14 3.16
---

Linus, can you please apply this?  Alternatively, Andrew, can you please 
pick this up and send it to Linus?

It would be good it it can be applied for 3.17 as well as to all stable 
kernels >= 3.6 as they are all broken!

Thanks a lot in advance!

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/

diff --git a/fs/buffer.c b/fs/buffer.c
index 8f05111..3588a80 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1022,7 +1022,8 @@ grow_dev_page(struct block_device *bdev, sector_t block,
bh = page_buffers(page);
if (bh->b_size == size) {
end_block = init_page_buffers(page, bdev,
-   index << sizebits, size);
+   (sector_t)index << sizebits,
+   size);
goto done;
}
if (!try_to_free_buffers(page))
@@ -1043,7 +1044,8 @@ grow_dev_page(struct block_device *bdev, sector_t block,
 */
spin_lock(>i_mapping->private_lock);
link_dev_buffers(page, bh);
-   end_block = init_page_buffers(page, bdev, index << sizebits, size);
+   end_block = init_page_buffers(page, bdev, (sector_t)index << sizebits,
+   size);
spin_unlock(>i_mapping->private_lock);
 done:
ret = (block < end_block) ? 1 : -ENXIO;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] Fix nasty 32-bit overflow bug in buffer i/o code.

2014-09-21 Thread Anton Altaparmakov

Any code that uses __getblk() and thus bread(), breadahead(), sb_bread(), 
sb_breadahead(), sb_getblk(), and calls it using a 64-bit block on a 
32-bit arch (where long is 32-bit) causes an inifinite loop in 
__getblk_slow() with an infinite stream of errors logged to dmesg like 
this:

__find_get_block_slow() failed. block=6740375944, b_blocknr=2445408648
b_state=0x0020, b_size=512
device sda1 blocksize: 512

Note how in hex block is 0x191C1F988 and b_blocknr is 0x91C1F988 i.e. the 
top 32-bits are missing (in this case the 0x1 at the top).

This is because grow_dev_page() was broken in commit 676ce6d5ca30: block: 
replace __getblk_slow misfix by grow_dev_page fix by Hugh Dickins so that 
it now has a 32-bit overflow due to shifting the block value to the right 
so it fits in 32-bits and storing the result in pgoff_t variable which is 
32-bit and then passing the pgoff_t variable left-shifted as the block 
number which causes the top bits to get lost as the pgoff_t is not type 
cast to sector_t / 64-bit before the shift.

This patch fixes this issue by type casting index to sector_t before 
doing the left shift.

Note this is not a theoretical bug but has been seen in the field on a 
4TiB hard drive with logical sector size 512 bytes.

This patch has been verified to fix the infinite loop problem on 3.17-rc5 
kernel using a 4TB disk image mounted using -o loop.  Without this patch 
doing a find /nt where /nt is an NTFS volume causes the inifinite loop 
100% reproducibly whilst with the patch it works fine as expected.

Signed-off-by: Anton Altaparmakov ai...@cantab.net
Cc: sta...@vger.kernel.org # 3.6 3.8 3.10 3.12 3.14 3.16
---

Linus, can you please apply this?  Alternatively, Andrew, can you please 
pick this up and send it to Linus?

It would be good it it can be applied for 3.17 as well as to all stable 
kernels = 3.6 as they are all broken!

Thanks a lot in advance!

Best regards,

Anton
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/

diff --git a/fs/buffer.c b/fs/buffer.c
index 8f05111..3588a80 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1022,7 +1022,8 @@ grow_dev_page(struct block_device *bdev, sector_t block,
bh = page_buffers(page);
if (bh-b_size == size) {
end_block = init_page_buffers(page, bdev,
-   index  sizebits, size);
+   (sector_t)index  sizebits,
+   size);
goto done;
}
if (!try_to_free_buffers(page))
@@ -1043,7 +1044,8 @@ grow_dev_page(struct block_device *bdev, sector_t block,
 */
spin_lock(inode-i_mapping-private_lock);
link_dev_buffers(page, bh);
-   end_block = init_page_buffers(page, bdev, index  sizebits, size);
+   end_block = init_page_buffers(page, bdev, (sector_t)index  sizebits,
+   size);
spin_unlock(inode-i_mapping-private_lock);
 done:
ret = (block  end_block) ? 1 : -ENXIO;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] Use find_get_page_flags() to mark page accessed as it is no longer marked later on

2014-09-05 Thread Anton Altaparmakov

Mel Gorman's commit 2457aec63745 ("mm: non-atomically mark page accessed 
during page cache allocation where possible") removed mark_page_accessed() 
calls from NTFS without updating the matching find_lock_page() to 
find_get_page_flags(GFP_LOCK | FGP_ACCESSED) thus causing the page to 
never be marked accessed.

This patch fixes that.

Signed-off-by: Anton Altaparmakov 
---

Hi Andrew,

Can you please take the below patch and forward it for inclusion to Linus?

Thanks a lot in advance!

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/

diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
index f5ec1ce..643faa4 100644
--- a/fs/ntfs/file.c
+++ b/fs/ntfs/file.c
@@ -1,7 +1,7 @@
 /*
  * file.c - NTFS kernel file operations.  Part of the Linux-NTFS project.
  *
- * Copyright (c) 2001-2011 Anton Altaparmakov and Tuxera Inc.
+ * Copyright (c) 2001-2014 Anton Altaparmakov and Tuxera Inc.
  *
  * This program/include file is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as published
@@ -410,7 +410,8 @@ static inline int __ntfs_grab_cache_pages(struct 
address_space *mapping,
BUG_ON(!nr_pages);
err = nr = 0;
do {
-   pages[nr] = find_lock_page(mapping, index);
+   pages[nr] = find_get_page_flags(mapping, index, FGP_LOCK |
+   FGP_ACCESSED);
if (!pages[nr]) {
if (!*cached_page) {
*cached_page = page_cache_alloc(mapping);
diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index 6c3296e..9e1e112 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -3208,7 +3208,7 @@ static void __exit exit_ntfs_fs(void)
 }
 
 MODULE_AUTHOR("Anton Altaparmakov ");
-MODULE_DESCRIPTION("NTFS 1.2/3.x driver - Copyright (c) 2001-2011 Anton 
Altaparmakov and Tuxera Inc.");
+MODULE_DESCRIPTION("NTFS 1.2/3.x driver - Copyright (c) 2001-2014 Anton 
Altaparmakov and Tuxera Inc.");
 MODULE_VERSION(NTFS_VERSION);
 MODULE_LICENSE("GPL");
 #ifdef DEBUG
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] Use find_get_page_flags() to mark page accessed as it is no longer marked later on

2014-09-05 Thread Anton Altaparmakov

Mel Gorman's commit 2457aec63745 (mm: non-atomically mark page accessed 
during page cache allocation where possible) removed mark_page_accessed() 
calls from NTFS without updating the matching find_lock_page() to 
find_get_page_flags(GFP_LOCK | FGP_ACCESSED) thus causing the page to 
never be marked accessed.

This patch fixes that.

Signed-off-by: Anton Altaparmakov an...@tuxera.com
---

Hi Andrew,

Can you please take the below patch and forward it for inclusion to Linus?

Thanks a lot in advance!

Best regards,

Anton
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/

diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
index f5ec1ce..643faa4 100644
--- a/fs/ntfs/file.c
+++ b/fs/ntfs/file.c
@@ -1,7 +1,7 @@
 /*
  * file.c - NTFS kernel file operations.  Part of the Linux-NTFS project.
  *
- * Copyright (c) 2001-2011 Anton Altaparmakov and Tuxera Inc.
+ * Copyright (c) 2001-2014 Anton Altaparmakov and Tuxera Inc.
  *
  * This program/include file is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as published
@@ -410,7 +410,8 @@ static inline int __ntfs_grab_cache_pages(struct 
address_space *mapping,
BUG_ON(!nr_pages);
err = nr = 0;
do {
-   pages[nr] = find_lock_page(mapping, index);
+   pages[nr] = find_get_page_flags(mapping, index, FGP_LOCK |
+   FGP_ACCESSED);
if (!pages[nr]) {
if (!*cached_page) {
*cached_page = page_cache_alloc(mapping);
diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index 6c3296e..9e1e112 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -3208,7 +3208,7 @@ static void __exit exit_ntfs_fs(void)
 }
 
 MODULE_AUTHOR(Anton Altaparmakov an...@tuxera.com);
-MODULE_DESCRIPTION(NTFS 1.2/3.x driver - Copyright (c) 2001-2011 Anton 
Altaparmakov and Tuxera Inc.);
+MODULE_DESCRIPTION(NTFS 1.2/3.x driver - Copyright (c) 2001-2014 Anton 
Altaparmakov and Tuxera Inc.);
 MODULE_VERSION(NTFS_VERSION);
 MODULE_LICENSE(GPL);
 #ifdef DEBUG
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Now that sync_filesystem() must be called from ->remount_fs() it needs to be EXPORT_SYMBOL() for kernel modules.

Hi Linus,

No-one has objected to the below patch I sent you over two weeks ago.

Would you please apply it?

If you would like it to go through for example Al or Andrew please let me 
know...

Thanks a lot in advance!

Best regards,

Anton

On 21 Aug 2014, at 11:09, Anton Altaparmakov  wrote:

> This patch changes sync_filesystem() to be EXPORT_SYMBOL().
> 
> The reason this is needed is that starting with 3.15 kernel, due to 
> Theodore Ts'o's commit 02b9984d640873b7b3809e63f81a0d7e13496886, "fs: push 
> sync_filesystem() down to the file system's remount_fs()", all file 
> systems that have dirty data to be written out need to call 
> sync_filesystem() from their ->remount_fs() method when remounting 
> read-only.
> 
> As this is now a generically required function rather than an internal 
> only function it should be EXPORT_SYMBOL() so that all file systems can
> call it.
> 
> Signed-off-by: Anton Altaparmakov 
> ---
> 
> Hi Linus,
> 
> Can you please apply this patch for inclusion into 3.17?  Explanation is 
> above.
> 
> Thanks a lot in advance!
> 
> PS. I hope Pine does not mess up the whitespace of the patch!
> 
> Best regards,
> 
>   Anton
> -- 
> Anton Altaparmakov  (replace at with @)
> University of Cambridge Information Services, Roger Needham Building
> 7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK
> 
> --- linux/fs/sync.c   2014-08-21 10:30:26.0 +0100
> +++ linux/fs/sync.c   2014-08-21 10:30:47.0 +0100
> @@ -65,7 +65,7 @@ int sync_filesystem(struct super_block *
>   return ret;
>   return __sync_filesystem(sb, 1);
> }
> -EXPORT_SYMBOL_GPL(sync_filesystem);
> +EXPORT_SYMBOL(sync_filesystem);
> 
> static void sync_inodes_one_sb(struct super_block *sb, void *arg)
> {

-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mm: clear __GFP_FS when PF_MEMALLOC_NOIO is set

On 4 Sep 2014, at 03:30, Andrew Morton  wrote:
> __GFP_FS and __GFP_IO are (or were) for communicating to vmscan: don't
> enter the fs for writepage, don't write back swapcache.
> 
> I guess those concepts have grown over time without a ton of thought
> going into it.  Yes, I suppose that if a filesystem's writepage is
> called (for example) it expects that it will be able to perform
> writeback and it won't check (or even be passed) the __GFP_IO setting.
> 
> So I guess we could say that !__GFP_FS && GFP_IO is not implemented and
> shouldn't occur.
> 
> That being said, it still seems quite bad to disable VFS cache
> shrinking for PF_MEMALLOC_NOIO allocation attempts.

I think what it really boils down to is that file systems cannot allow 
recursion into _that_ file system so if VFS/VM shrinking could skip over all 
inodes/dentries/pages that are associated with the superblock of the volume for 
which the allocation is being done then that would be just fine.

An alternative would be that the file systems would need to be passed in a flag 
that will tell them that it is not safe to take locks and then file systems 
that need to take a lock could return with -EDEADLOCK and the VM can then skip 
over those entries and reclaim others.  Though I think it would be more 
efficient for the VFS/VM to simply not call into the file system that is doing 
the allocation as above...

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Now that sync_filesystem() must be called from -remount_fs() it needs to be EXPORT_SYMBOL() for kernel modules.

Hi Linus,

No-one has objected to the below patch I sent you over two weeks ago.

Would you please apply it?

If you would like it to go through for example Al or Andrew please let me 
know...

Thanks a lot in advance!

Best regards,

Anton

On 21 Aug 2014, at 11:09, Anton Altaparmakov ai...@cam.ac.uk wrote:

 This patch changes sync_filesystem() to be EXPORT_SYMBOL().
 
 The reason this is needed is that starting with 3.15 kernel, due to 
 Theodore Ts'o's commit 02b9984d640873b7b3809e63f81a0d7e13496886, fs: push 
 sync_filesystem() down to the file system's remount_fs(), all file 
 systems that have dirty data to be written out need to call 
 sync_filesystem() from their -remount_fs() method when remounting 
 read-only.
 
 As this is now a generically required function rather than an internal 
 only function it should be EXPORT_SYMBOL() so that all file systems can
 call it.
 
 Signed-off-by: Anton Altaparmakov ai...@cantab.net
 ---
 
 Hi Linus,
 
 Can you please apply this patch for inclusion into 3.17?  Explanation is 
 above.
 
 Thanks a lot in advance!
 
 PS. I hope Pine does not mess up the whitespace of the patch!
 
 Best regards,
 
   Anton
 -- 
 Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
 University of Cambridge Information Services, Roger Needham Building
 7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK
 
 --- linux/fs/sync.c   2014-08-21 10:30:26.0 +0100
 +++ linux/fs/sync.c   2014-08-21 10:30:47.0 +0100
 @@ -65,7 +65,7 @@ int sync_filesystem(struct super_block *
   return ret;
   return __sync_filesystem(sb, 1);
 }
 -EXPORT_SYMBOL_GPL(sync_filesystem);
 +EXPORT_SYMBOL(sync_filesystem);
 
 static void sync_inodes_one_sb(struct super_block *sb, void *arg)
 {

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH] mm: clear __GFP_FS when PF_MEMALLOC_NOIO is set

On 4 Sep 2014, at 03:30, Andrew Morton a...@linux-foundation.org wrote:
 __GFP_FS and __GFP_IO are (or were) for communicating to vmscan: don't
 enter the fs for writepage, don't write back swapcache.
 
 I guess those concepts have grown over time without a ton of thought
 going into it.  Yes, I suppose that if a filesystem's writepage is
 called (for example) it expects that it will be able to perform
 writeback and it won't check (or even be passed) the __GFP_IO setting.
 
 So I guess we could say that !__GFP_FS  GFP_IO is not implemented and
 shouldn't occur.
 
 That being said, it still seems quite bad to disable VFS cache
 shrinking for PF_MEMALLOC_NOIO allocation attempts.

I think what it really boils down to is that file systems cannot allow 
recursion into _that_ file system so if VFS/VM shrinking could skip over all 
inodes/dentries/pages that are associated with the superblock of the volume for 
which the allocation is being done then that would be just fine.

An alternative would be that the file systems would need to be passed in a flag 
that will tell them that it is not safe to take locks and then file systems 
that need to take a lock could return with -EDEADLOCK and the VM can then skip 
over those entries and reclaim others.  Though I think it would be more 
efficient for the VFS/VM to simply not call into the file system that is doing 
the allocation as above...

Best regards,

Anton
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

[PATCH] Now that sync_filesystem() must be called from ->remount_fs() it needs to be EXPORT_SYMBOL() for kernel modules.

2014-08-21 Thread Anton Altaparmakov

This patch changes sync_filesystem() to be EXPORT_SYMBOL().

The reason this is needed is that starting with 3.15 kernel, due to 
Theodore Ts'o's commit 02b9984d640873b7b3809e63f81a0d7e13496886, "fs: push 
sync_filesystem() down to the file system's remount_fs()", all file 
systems that have dirty data to be written out need to call 
sync_filesystem() from their ->remount_fs() method when remounting 
read-only.

As this is now a generically required function rather than an internal 
only function it should be EXPORT_SYMBOL() so that all file systems can
call it.

Signed-off-by: Anton Altaparmakov 
---

Hi Linus,

Can you please apply this patch for inclusion into 3.17?  Explanation is 
above.

Thanks a lot in advance!

PS. I hope Pine does not mess up the whitespace of the patch!

Best regards,

    Anton
-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--- linux/fs/sync.c 2014-08-21 10:30:26.0 +0100
+++ linux/fs/sync.c 2014-08-21 10:30:47.0 +0100
@@ -65,7 +65,7 @@ int sync_filesystem(struct super_block *
return ret;
return __sync_filesystem(sb, 1);
 }
-EXPORT_SYMBOL_GPL(sync_filesystem);
+EXPORT_SYMBOL(sync_filesystem);
 
 static void sync_inodes_one_sb(struct super_block *sb, void *arg)
 {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] Now that sync_filesystem() must be called from -remount_fs() it needs to be EXPORT_SYMBOL() for kernel modules.

2014-08-21 Thread Anton Altaparmakov

This patch changes sync_filesystem() to be EXPORT_SYMBOL().

The reason this is needed is that starting with 3.15 kernel, due to 
Theodore Ts'o's commit 02b9984d640873b7b3809e63f81a0d7e13496886, fs: push 
sync_filesystem() down to the file system's remount_fs(), all file 
systems that have dirty data to be written out need to call 
sync_filesystem() from their -remount_fs() method when remounting 
read-only.

As this is now a generically required function rather than an internal 
only function it should be EXPORT_SYMBOL() so that all file systems can
call it.

Signed-off-by: Anton Altaparmakov ai...@cantab.net
---

Hi Linus,

Can you please apply this patch for inclusion into 3.17?  Explanation is 
above.

Thanks a lot in advance!

PS. I hope Pine does not mess up the whitespace of the patch!

Best regards,

Anton
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--- linux/fs/sync.c 2014-08-21 10:30:26.0 +0100
+++ linux/fs/sync.c 2014-08-21 10:30:47.0 +0100
@@ -65,7 +65,7 @@ int sync_filesystem(struct super_block *
return ret;
return __sync_filesystem(sb, 1);
 }
-EXPORT_SYMBOL_GPL(sync_filesystem);
+EXPORT_SYMBOL(sync_filesystem);
 
 static void sync_inodes_one_sb(struct super_block *sb, void *arg)
 {
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Revert "platform/x86/toshiba-apci.c possible bad if test?"

2014-08-20 Thread Anton Altaparmakov

Hi Matthew,

There is no doubt that the revert was needed but I think the original check 
which is now reinstated is also wrong.

I do not know what the intention actually is so cannot say what the correct 
check is but just logically speaking the check makes no sense:

if (sscanf(buf, "%i", ) != 1 && (mode != 2 || mode != 1))

The condition "(mode != 2 || mode != 1)" will always be true given that mode 
can only have a single value: if mode == 1 then mode != 2 is true and thus the 
condition is true and vice versa if mode == 2 then mode != 1 is true and thus 
the condition is true, too.  And if mode is neither 1 nor 2 then both mode != 2 
and mode != 1 are true and thus the condition is true, too.  Thus no matter 
what value "mode" has, the condition is true thus there is no point in it being 
there thus the if clause is exactly the same as:

if (sscanf(buf, "%i", ) != 1)

Presumably that was not the original intention...

Perhaps the intention was to check that mode is either 1 or 2 and other values 
are not allowed?  If so the correct statement would be:

if (sscanf(buf, "%i", ) != 1 || (mode != 2 && mode != 1))

But as I said above I do not know if that was the original intention but it 
looks like that this may have been the intention...

Best regards,

Anton

On 21 Aug 2014, at 00:53, Linux Kernel Mailing List 
 wrote:

> Gitweb: 
> http://git.kernel.org/linus/;a=commit;h=8039aabb6c9f802bca04cc77ca210060a5b53916
> Commit: 8039aabb6c9f802bca04cc77ca210060a5b53916
> Parent: 186e4e89a0922d75fba476f15b723e541cc34bea
> Refname:refs/heads/master
> Author: Matthew Garrett 
> AuthorDate: Wed Aug 20 08:18:18 2014 -0700
> Committer:  Matthew Garrett 
> CommitDate: Wed Aug 20 08:18:18 2014 -0700
> 
>Revert "platform/x86/toshiba-apci.c possible bad if test?"
> 
>This reverts commit bdc3ae7221213963f438faeaa69c8b4a2195f491.
> 
>Signed-off-by: Matthew Garrett 
> ---
> drivers/platform/x86/toshiba_acpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c 
> b/drivers/platform/x86/toshiba_acpi.c
> index e4da61b..b062d3d 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -1258,7 +1258,7 @@ static ssize_t toshiba_kbd_bl_mode_store(struct device 
> *dev,
>   int mode = -1;
>   int time = -1;
> 
> - if (sscanf(buf, "%i", ) != 1  || (mode != 2 || mode != 1))
> + if (sscanf(buf, "%i", ) != 1 && (mode != 2 || mode != 1))
>   return -EINVAL;
> 
>   /* Set the Keyboard Backlight Mode where:
-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Revert platform/x86/toshiba-apci.c possible bad if test?

2014-08-20 Thread Anton Altaparmakov

Hi Matthew,

There is no doubt that the revert was needed but I think the original check 
which is now reinstated is also wrong.

I do not know what the intention actually is so cannot say what the correct 
check is but just logically speaking the check makes no sense:

if (sscanf(buf, %i, mode) != 1  (mode != 2 || mode != 1))

The condition (mode != 2 || mode != 1) will always be true given that mode 
can only have a single value: if mode == 1 then mode != 2 is true and thus the 
condition is true and vice versa if mode == 2 then mode != 1 is true and thus 
the condition is true, too.  And if mode is neither 1 nor 2 then both mode != 2 
and mode != 1 are true and thus the condition is true, too.  Thus no matter 
what value mode has, the condition is true thus there is no point in it being 
there thus the if clause is exactly the same as:

if (sscanf(buf, %i, mode) != 1)

Presumably that was not the original intention...

Perhaps the intention was to check that mode is either 1 or 2 and other values 
are not allowed?  If so the correct statement would be:

if (sscanf(buf, %i, mode) != 1 || (mode != 2  mode != 1))

But as I said above I do not know if that was the original intention but it 
looks like that this may have been the intention...

Best regards,

Anton

On 21 Aug 2014, at 00:53, Linux Kernel Mailing List 
linux-kernel@vger.kernel.org wrote:

 Gitweb: 
 http://git.kernel.org/linus/;a=commit;h=8039aabb6c9f802bca04cc77ca210060a5b53916
 Commit: 8039aabb6c9f802bca04cc77ca210060a5b53916
 Parent: 186e4e89a0922d75fba476f15b723e541cc34bea
 Refname:refs/heads/master
 Author: Matthew Garrett matthew.garr...@nebula.com
 AuthorDate: Wed Aug 20 08:18:18 2014 -0700
 Committer:  Matthew Garrett matthew.garr...@nebula.com
 CommitDate: Wed Aug 20 08:18:18 2014 -0700
 
Revert platform/x86/toshiba-apci.c possible bad if test?
 
This reverts commit bdc3ae7221213963f438faeaa69c8b4a2195f491.
 
Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
 ---
 drivers/platform/x86/toshiba_acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/platform/x86/toshiba_acpi.c 
 b/drivers/platform/x86/toshiba_acpi.c
 index e4da61b..b062d3d 100644
 --- a/drivers/platform/x86/toshiba_acpi.c
 +++ b/drivers/platform/x86/toshiba_acpi.c
 @@ -1258,7 +1258,7 @@ static ssize_t toshiba_kbd_bl_mode_store(struct device 
 *dev,
   int mode = -1;
   int time = -1;
 
 - if (sscanf(buf, %i, mode) != 1  || (mode != 2 || mode != 1))
 + if (sscanf(buf, %i, mode) != 1  (mode != 2 || mode != 1))
   return -EINVAL;
 
   /* Set the Keyboard Backlight Mode where:
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: Bug introduced in 3b93f911d5

2014-08-08 Thread Anton Altaparmakov

Hi Al,

On 8 Aug 2014, at 16:54, Al Viro  wrote:
> On Fri, Aug 08, 2014 at 12:11:39AM +0100, Anton Altaparmakov wrote:
>> Hi Al,
>> 
>> Was just looking at __generic_file_write_iter() and found a bug in the code 
>> that you added in 3b93f911d5.
>> 
>> Consider the case where generic_file_direct_write() returns a partial write, 
>> i.e. written > 0 && written < count.
>> 
>> Also consider that the following generic_perform_write() fails with an 
>> error, i.e. status < 0.
> 
> *nod*
> 
> What we ought to do, AFAICS, is this:
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 900edfa..8163e04 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2584,7 +2584,7 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, 
> struct iov_iter *from)
>* that this differs from normal direct-io semantics, which
>* will return -EFOO even if some bytes were written.
>*/
> - if (unlikely(status < 0) && !written) {
> + if (unlikely(status < 0)) {
>   err = status;
>   goto out;
>   }
> 
> Note that we return written ? written : err, so assignment to err will be
> the right thing both when status < 0 && written == 0 and when status < 0 &&
> written > 0.  In the latter case err will be simply ignored.
> 
> Objections?

No objections from me.  As you say, that will do the right thing in all cases.

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Bug introduced in 3b93f911d5

2014-08-08 Thread Anton Altaparmakov

Hi Al,

On 8 Aug 2014, at 16:54, Al Viro v...@zeniv.linux.org.uk wrote:
 On Fri, Aug 08, 2014 at 12:11:39AM +0100, Anton Altaparmakov wrote:
 Hi Al,
 
 Was just looking at __generic_file_write_iter() and found a bug in the code 
 that you added in 3b93f911d5.
 
 Consider the case where generic_file_direct_write() returns a partial write, 
 i.e. written  0  written  count.
 
 Also consider that the following generic_perform_write() fails with an 
 error, i.e. status  0.
 
 *nod*
 
 What we ought to do, AFAICS, is this:
 
 diff --git a/mm/filemap.c b/mm/filemap.c
 index 900edfa..8163e04 100644
 --- a/mm/filemap.c
 +++ b/mm/filemap.c
 @@ -2584,7 +2584,7 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, 
 struct iov_iter *from)
* that this differs from normal direct-io semantics, which
* will return -EFOO even if some bytes were written.
*/
 - if (unlikely(status  0)  !written) {
 + if (unlikely(status  0)) {
   err = status;
   goto out;
   }
 
 Note that we return written ? written : err, so assignment to err will be
 the right thing both when status  0  written == 0 and when status  0 
 written  0.  In the latter case err will be simply ignored.
 
 Objections?

No objections from me.  As you say, that will do the right thing in all cases.

Best regards,

Anton
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Bug introduced in 3b93f911d5

2014-08-07 Thread Anton Altaparmakov

Hi Al,

Was just looking at __generic_file_write_iter() and found a bug in the code 
that you added in 3b93f911d5.

Consider the case where generic_file_direct_write() returns a partial write, 
i.e. written > 0 && written < count.

Also consider that the following generic_perform_write() fails with an error, 
i.e. status < 0.

This code then does something very bogus:

if (unlikely(status < 0) && !written) {
err = status;
goto out;
}
iocb->ki_pos = pos + status;
...
endbyte = pos + status - 1;

The if condition is false as written is > 0 yet status is negative thus 
iocb->ki_pos is set to pos + status where status is negative thus ki_pos is 
actually set to "pos - random value".

And similar for "endbyte" being set to "pos - random value - 1", etc.

Doesn't seem like that is what you intended?

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Bug introduced in 3b93f911d5

2014-08-07 Thread Anton Altaparmakov

Hi Al,

Was just looking at __generic_file_write_iter() and found a bug in the code 
that you added in 3b93f911d5.

Consider the case where generic_file_direct_write() returns a partial write, 
i.e. written  0  written  count.

Also consider that the following generic_perform_write() fails with an error, 
i.e. status  0.

This code then does something very bogus:

if (unlikely(status  0)  !written) {
err = status;
goto out;
}
iocb-ki_pos = pos + status;
...
endbyte = pos + status - 1;

The if condition is false as written is  0 yet status is negative thus 
iocb-ki_pos is set to pos + status where status is negative thus ki_pos is 
actually set to pos - random value.

And similar for endbyte being set to pos - random value - 1, etc.

Doesn't seem like that is what you intended?

Best regards,

Anton
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH] ntfs: avoid incorrectly release for root inode in fill_super

Hi,

NAK

This patch is incorrect.  Perhaps you failed to see the ihold() above the 
d_make_root() call?  That means we hold two references on the inode - one from 
the load_system_files()::ntfs_iget() and one from the ihold() before 
d_make_root().

Thus in the error code path d_make_root() does iput() which releases one 
reference and then we do iput() in the error handling path of ntfs_fill_super() 
which releases the second reference.

Best regards,

Anton

On 25 Jul 2014, at 03:25, Chao Yu  wrote:

> In d_make_root, when we fail to allocate dentry for root inode, we will iput
> root inode in this function.
> So we do not need to release this inode again at d_make_root's caller.
> 
> Signed-off-by: Chao Yu 
> ---
> fs/ntfs/super.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index 6c3296e..99c5cc6 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -2975,7 +2975,11 @@ static int ntfs_fill_super(struct super_block *sb, 
> void *opt, const int silent)
>   vol->secure_ino = NULL;
>   }
>   }
> - iput(vol->root_ino);
> +
> + /*
> +  * Just set NULL value here because we have already iput root_ino
> +  * in d_make_root.
> +  */
>   vol->root_ino = NULL;
>   iput(vol->lcnbmp_ino);
>   vol->lcnbmp_ino = NULL;

-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux fsync behaviour

Hi,

On 25 Jul 2014, at 02:17, Dave Chinner  wrote:
> On Thu, Jul 24, 2014 at 03:41:31PM -0700, yuanh wrote:
>> Hi all,
>> 
>>   Two file descriptors are pointing the same file. When fsync is called on
>> one fd, the data written by the other fd will also be flushed? We are using
>> linux XFS.
> 
> Yes.

But beware of the common mistake of using fwrite + fsync which does not 
actually do what you intend at all and you must instead use fwrite + fflush + 
fsync...  If you are using write(2) then just fsync is obviously fine.

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux fsync behaviour

Hi,

On 25 Jul 2014, at 02:17, Dave Chinner da...@fromorbit.com wrote:
 On Thu, Jul 24, 2014 at 03:41:31PM -0700, yuanh wrote:
 Hi all,
 
   Two file descriptors are pointing the same file. When fsync is called on
 one fd, the data written by the other fd will also be flushed? We are using
 linux XFS.
 
 Yes.

But beware of the common mistake of using fwrite + fsync which does not 
actually do what you intend at all and you must instead use fwrite + fflush + 
fsync...  If you are using write(2) then just fsync is obviously fine.

Best regards,

Anton
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH] ntfs: avoid incorrectly release for root inode in fill_super

Hi,

NAK

This patch is incorrect.  Perhaps you failed to see the ihold() above the 
d_make_root() call?  That means we hold two references on the inode - one from 
the load_system_files()::ntfs_iget() and one from the ihold() before 
d_make_root().

Thus in the error code path d_make_root() does iput() which releases one 
reference and then we do iput() in the error handling path of ntfs_fill_super() 
which releases the second reference.

Best regards,

Anton

On 25 Jul 2014, at 03:25, Chao Yu chao2...@samsung.com wrote:

 In d_make_root, when we fail to allocate dentry for root inode, we will iput
 root inode in this function.
 So we do not need to release this inode again at d_make_root's caller.
 
 Signed-off-by: Chao Yu chao2...@samsung.com
 ---
 fs/ntfs/super.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)
 
 diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
 index 6c3296e..99c5cc6 100644
 --- a/fs/ntfs/super.c
 +++ b/fs/ntfs/super.c
 @@ -2975,7 +2975,11 @@ static int ntfs_fill_super(struct super_block *sb, 
 void *opt, const int silent)
   vol-secure_ino = NULL;
   }
   }
 - iput(vol-root_ino);
 +
 + /*
 +  * Just set NULL value here because we have already iput root_ino
 +  * in d_make_root.
 +  */
   vol-root_ino = NULL;
   iput(vol-lcnbmp_ino);
   vol-lcnbmp_ino = NULL;

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH 1/1] ntfs: kernel-doc warning fixes

2014-07-04 Thread Anton Altaparmakov

Hi,

Looks good, thanks.  Feel free to add:

Acked-by: Anton Altaparmakov 

Andrew, could you please take it in your tree for merging?

Thanks a lot in advance!

Best regards,

Anton

On 4 Jul 2014, at 20:53, Fabian Frederick  wrote:

> cached_page and lru_pvec were removed from
> ntfs_attr_extend_initialized in commit 2ec93b0bf35f
> ("ntfs: clean up ntfs_attr_extend_initialized")
> 
> lru_pvec has been removed from __ntfs_grab_cache_pages in commit 4c99000ac47c
> ("ntfs: use add_to_page_cache_lru()")
> 
> Cc: Anton Altaparmakov 
> Cc: Andrew Morton 
> Cc: linux-ntfs-...@lists.sourceforge.net
> Signed-off-by: Fabian Frederick 
> ---
> fs/ntfs/file.c | 3 ---
> 1 file changed, 3 deletions(-)
> 
> diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
> index 5c9e2c8..f5ec1ce 100644
> --- a/fs/ntfs/file.c
> +++ b/fs/ntfs/file.c
> @@ -74,8 +74,6 @@ static int ntfs_file_open(struct inode *vi, struct file 
> *filp)
>  * ntfs_attr_extend_initialized - extend the initialized size of an attribute
>  * @ni:   ntfs inode of the attribute to extend
>  * @new_init_size:requested new initialized size in bytes
> - * @cached_page: store any allocated but unused page here
> - * @lru_pvec:lru-buffering pagevec of the caller
>  *
>  * Extend the initialized size of an attribute described by the ntfs inode @ni
>  * to @new_init_size bytes.  This involves zeroing any non-sparse space 
> between
> @@ -395,7 +393,6 @@ static inline void 
> ntfs_fault_in_pages_readable_iovec(const struct iovec *iov,
>  * @nr_pages: number of page cache pages to obtain
>  * @pages:array of pages in which to return the obtained page cache pages
>  * @cached_page: allocated but as yet unused page
> - * @lru_pvec:lru-buffering pagevec of caller
>  *
>  * Obtain @nr_pages locked page cache pages from the mapping @mapping and
>  * starting at index @index.
> -- 
> 1.9.1
> 

-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] ntfs: kernel-doc warning fixes

2014-07-04 Thread Anton Altaparmakov

Hi,

Looks good, thanks.  Feel free to add:

Acked-by: Anton Altaparmakov an...@tuxera.com

Andrew, could you please take it in your tree for merging?

Thanks a lot in advance!

Best regards,

Anton

On 4 Jul 2014, at 20:53, Fabian Frederick f...@skynet.be wrote:

 cached_page and lru_pvec were removed from
 ntfs_attr_extend_initialized in commit 2ec93b0bf35f
 (ntfs: clean up ntfs_attr_extend_initialized)
 
 lru_pvec has been removed from __ntfs_grab_cache_pages in commit 4c99000ac47c
 (ntfs: use add_to_page_cache_lru())
 
 Cc: Anton Altaparmakov an...@tuxera.com
 Cc: Andrew Morton a...@linux-foundation.org
 Cc: linux-ntfs-...@lists.sourceforge.net
 Signed-off-by: Fabian Frederick f...@skynet.be
 ---
 fs/ntfs/file.c | 3 ---
 1 file changed, 3 deletions(-)
 
 diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
 index 5c9e2c8..f5ec1ce 100644
 --- a/fs/ntfs/file.c
 +++ b/fs/ntfs/file.c
 @@ -74,8 +74,6 @@ static int ntfs_file_open(struct inode *vi, struct file 
 *filp)
  * ntfs_attr_extend_initialized - extend the initialized size of an attribute
  * @ni:   ntfs inode of the attribute to extend
  * @new_init_size:requested new initialized size in bytes
 - * @cached_page: store any allocated but unused page here
 - * @lru_pvec:lru-buffering pagevec of the caller
  *
  * Extend the initialized size of an attribute described by the ntfs inode @ni
  * to @new_init_size bytes.  This involves zeroing any non-sparse space 
 between
 @@ -395,7 +393,6 @@ static inline void 
 ntfs_fault_in_pages_readable_iovec(const struct iovec *iov,
  * @nr_pages: number of page cache pages to obtain
  * @pages:array of pages in which to return the obtained page cache pages
  * @cached_page: allocated but as yet unused page
 - * @lru_pvec:lru-buffering pagevec of caller
  *
  * Obtain @nr_pages locked page cache pages from the mapping @mapping and
  * starting at index @index.
 -- 
 1.9.1
 

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH] ntfs: Drop cast

2014-06-28 Thread Anton Altaparmakov

Hi,

Thanks for your patch.  Your patch is obviously correct however I actually 
prefer to have the type casts in place because it clearly shows what the 
variable type being assigned to is so I find it makes the code more readable to 
cast void pointers to the target variable type so I don't have to go and look 
up the type higher up.  Having said that I don't feel that strongly about it so 
I have no real objection to the patch getting applied...

Best regards,

Anton

On 28 Jun 2014, at 14:33, Himangi Saraogi  wrote:

> This patch removes the cast on data of type void * as it is not needed.
> The following Coccinelle semantic patch was used for making the change:
> 
> @r@
> expression x;
> void* e;
> type T;
> identifier f;
> @@
> 
> (
>  *((T *)e)
> |
>  ((T *)x)[...]
> |
>  ((T *)x)->f
> |
> - (T *)
>  e
> )
> 
> Signed-off-by: Himangi Saraogi 
> Acked-by: Julia Lawall 
> ---
> fs/ntfs/inode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index f47af5e..b3c55fc 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -1817,7 +1817,7 @@ int ntfs_read_inode_mount(struct inode *vi)
>   i = vol->mft_record_size;
>   if (i < sb->s_blocksize)
>   i = sb->s_blocksize;
> - m = (MFT_RECORD*)ntfs_malloc_nofs(i);
> + m = ntfs_malloc_nofs(i);
>   if (!m) {
>   ntfs_error(sb, "Failed to allocate buffer for $MFT record 0.");
>   goto err_out;

-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ntfs: Drop cast

2014-06-28 Thread Anton Altaparmakov

Hi,

Thanks for your patch.  Your patch is obviously correct however I actually 
prefer to have the type casts in place because it clearly shows what the 
variable type being assigned to is so I find it makes the code more readable to 
cast void pointers to the target variable type so I don't have to go and look 
up the type higher up.  Having said that I don't feel that strongly about it so 
I have no real objection to the patch getting applied...

Best regards,

Anton

On 28 Jun 2014, at 14:33, Himangi Saraogi himangi...@gmail.com wrote:

 This patch removes the cast on data of type void * as it is not needed.
 The following Coccinelle semantic patch was used for making the change:
 
 @r@
 expression x;
 void* e;
 type T;
 identifier f;
 @@
 
 (
  *((T *)e)
 |
  ((T *)x)[...]
 |
  ((T *)x)-f
 |
 - (T *)
  e
 )
 
 Signed-off-by: Himangi Saraogi himangi...@gmail.com
 Acked-by: Julia Lawall julia.law...@lip6.fr
 ---
 fs/ntfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
 index f47af5e..b3c55fc 100644
 --- a/fs/ntfs/inode.c
 +++ b/fs/ntfs/inode.c
 @@ -1817,7 +1817,7 @@ int ntfs_read_inode_mount(struct inode *vi)
   i = vol-mft_record_size;
   if (i  sb-s_blocksize)
   i = sb-s_blocksize;
 - m = (MFT_RECORD*)ntfs_malloc_nofs(i);
 + m = ntfs_malloc_nofs(i);
   if (!m) {
   ntfs_error(sb, Failed to allocate buffer for $MFT record 0.);
   goto err_out;

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH] ntfs: Cleanup string initializations (char[] instead of char *)

2014-05-17 Thread Anton Altaparmakov

Hi,

NAK.

As Al Viro said in reply to one of your other patches, if you look at the 
assembler output you will see that the 'char te[] = "string"' generates 
dreadful code whilst the 'char *te = "string"' generates much better code thus 
if anything we should change all occurrences of 'char (.*)\[\] =' to "char *\1" 
instead...

If you think about it logically it actually makes sense, too.  In the 'char 
te[] = "string"' case we are telling the compiler to take a constant string 
"string" which will be stored both in the binary and once loaded in memory, 
then make a copy of it into the array "te" and that is exactly what the 
compiler does (if the string is short enough the compiler actually copies 
immediate values instead of storing the string elsewhere but that is still 
worse than just pointing to constant memory).  Whilst in the 'char *te = 
"string"' case we are telling the compiler to take a constant string (as 
previous case) and then assign the address of that string to the pointer 
variable "te".  Thus no copying takes place.  Thus one should never use the 
'char te[] = "string"' form UNLESS you want to then mess about with the 
contents of the string in which case modifying "te[]" will work as it is a 
local copy but modifying "*te" will cause a crash or be ignored depending on 
whether you are programming in the kernel or user space because you are trying 
to modify a read-only memory segment...

Best regards,

Anton

On 17 May 2014, at 13:25, Manuel Schölling  wrote:

> Initializations like 'char *foo = "bar"' will create two variables: a static
> string and a pointer (foo) to that static string. Instead 'char foo[] = "bar"'
> will allocate only a declare a single variable and will end up in shorter
> assembly (according to Jeff Garzik on the KernelJanitor's TODO list).
> 
> Signed-off-by: Manuel Schölling 
> ---
> fs/ntfs/inode.c |2 +-
> fs/ntfs/super.c |2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index f47af5e..3975798 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -2368,7 +2368,7 @@ int ntfs_truncate(struct inode *vi)
>   ntfs_attr_search_ctx *ctx;
>   MFT_RECORD *m;
>   ATTR_RECORD *a;
> - const char *te = "  Leaving file length out of sync with i_size.";
> + const char te[] = "  Leaving file length out of sync with i_size.";
>   int err, mp_size, size_change, alloc_change;
>   u32 attr_len;
> 
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index 9de2491..eb2c195 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -676,7 +676,7 @@ not_ntfs:
> static struct buffer_head *read_ntfs_boot_sector(struct super_block *sb,
>   const int silent)
> {
> - const char *read_err_str = "Unable to read %s boot sector.";
> + const char read_err_str[] = "Unable to read %s boot sector.";
>   struct buffer_head *bh_primary, *bh_backup;
>   sector_t nr_blocks = NTFS_SB(sb)->nr_blocks;
> 
> -- 
> 1.7.10.4

-- 
Anton Altaparmakov  (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ntfs: Cleanup string initializations (char[] instead of char *)

2014-05-17 Thread Anton Altaparmakov

Hi,

NAK.

As Al Viro said in reply to one of your other patches, if you look at the 
assembler output you will see that the 'char te[] = string' generates 
dreadful code whilst the 'char *te = string' generates much better code thus 
if anything we should change all occurrences of 'char (.*)\[\] =' to char *\1 
instead...

If you think about it logically it actually makes sense, too.  In the 'char 
te[] = string' case we are telling the compiler to take a constant string 
string which will be stored both in the binary and once loaded in memory, 
then make a copy of it into the array te and that is exactly what the 
compiler does (if the string is short enough the compiler actually copies 
immediate values instead of storing the string elsewhere but that is still 
worse than just pointing to constant memory).  Whilst in the 'char *te = 
string' case we are telling the compiler to take a constant string (as 
previous case) and then assign the address of that string to the pointer 
variable te.  Thus no copying takes place.  Thus one should never use the 
'char te[] = string' form UNLESS you want to then mess about with the 
contents of the string in which case modifying te[] will work as it is a 
local copy but modifying *te will cause a crash or be ignored depending on 
whether you are programming in the kernel or user space because you are trying 
to modify a read-only memory segment...

Best regards,

Anton

On 17 May 2014, at 13:25, Manuel Schölling manuel.schoell...@gmx.de wrote:

 Initializations like 'char *foo = bar' will create two variables: a static
 string and a pointer (foo) to that static string. Instead 'char foo[] = bar'
 will allocate only a declare a single variable and will end up in shorter
 assembly (according to Jeff Garzik on the KernelJanitor's TODO list).
 
 Signed-off-by: Manuel Schölling manuel.schoell...@gmx.de
 ---
 fs/ntfs/inode.c |2 +-
 fs/ntfs/super.c |2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
 index f47af5e..3975798 100644
 --- a/fs/ntfs/inode.c
 +++ b/fs/ntfs/inode.c
 @@ -2368,7 +2368,7 @@ int ntfs_truncate(struct inode *vi)
   ntfs_attr_search_ctx *ctx;
   MFT_RECORD *m;
   ATTR_RECORD *a;
 - const char *te =   Leaving file length out of sync with i_size.;
 + const char te[] =   Leaving file length out of sync with i_size.;
   int err, mp_size, size_change, alloc_change;
   u32 attr_len;
 
 diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
 index 9de2491..eb2c195 100644
 --- a/fs/ntfs/super.c
 +++ b/fs/ntfs/super.c
 @@ -676,7 +676,7 @@ not_ntfs:
 static struct buffer_head *read_ntfs_boot_sector(struct super_block *sb,
   const int silent)
 {
 - const char *read_err_str = Unable to read %s boot sector.;
 + const char read_err_str[] = Unable to read %s boot sector.;
   struct buffer_head *bh_primary, *bh_backup;
   sector_t nr_blocks = NTFS_SB(sb)-nr_blocks;
 
 -- 
 1.7.10.4

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH 1/1] fs/ntfs/mft.c: fix 2 sparse warnings

2014-04-28 Thread Anton Altaparmakov

NAK.

I do not see why this is needed.  Not all sparse warnings need fixing.

What is so bad about a variable length array that we need to incur kmalloc 
overhead on each call?  I would have thought the CPU overhead of the kmalloc is 
a lot larger than any CPU overhead of a variable length array...

The array size is tiny - basically on all volumes mft_record_size is 1024 and 
blocksize is either 512 or 1024 thus the array size is either 1 or 2 pointers.  
The reason it is dynamic is just in case Microsoft change their mind and 
increase the mft_record_size in a future NTFS/Windows version to something 
bigger than 1024 bytes, the current code would still cope fine.

Note mft_record_size is the "on disk inode size" thus it will never be much 
bigger and unlikely to ever change from the 1024 really.  A very long time ago 
when NTFS was being developed Microsoft used 2048 bytes but quickly changed it 
to 1024 as most inodes do not need anywhere near that much space for an inode.

Best regards,

Anton

On 27 Apr 2014, at 11:12, Fabian Frederick  wrote:

> fs/ntfs/mft.c:471:33: warning: Variable length array is used.
> fs/ntfs/mft.c:676:33: warning: Variable length array is used.
> 
> This is untested.
> 
> Cc: Anton Altaparmakov 
> Cc: Andrew Morton 
> Signed-off-by: Fabian Frederick 
> ---
> fs/ntfs/mft.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
> index 3014a36..eddb739 100644
> --- a/fs/ntfs/mft.c
> +++ b/fs/ntfs/mft.c
> @@ -468,7 +468,7 @@ int ntfs_sync_mft_mirror(ntfs_volume *vol, const unsigned 
> long mft_no,
>   struct page *page;
>   unsigned int blocksize = vol->sb->s_blocksize;
>   int max_bhs = vol->mft_record_size / blocksize;
> - struct buffer_head *bhs[max_bhs];
> + struct buffer_head **bhs;
>   struct buffer_head *bh, *head;
>   u8 *kmirr;
>   runlist_element *rl;
> @@ -478,11 +478,14 @@ int ntfs_sync_mft_mirror(ntfs_volume *vol, const 
> unsigned long mft_no,
> 
>   ntfs_debug("Entering for inode 0x%lx.", mft_no);
>   BUG_ON(!max_bhs);
> - if (unlikely(!vol->mftmirr_ino)) {
> + bhs = kmalloc(max_bhs * sizeof(struct buffer_head *), GFP_NOFS);
> + if (unlikely(!bhs || !vol->mftmirr_ino)) {
>   /* This could happen during umount... */
>   err = ntfs_sync_mft_mirror_umount(vol, mft_no, m);
> - if (likely(!err))
> + if (likely(!err)) {
> + kfree(bhs);
>   return err;
> + }
>   goto err_out;
>   }
>   /* Get the page containing the mirror copy of the mft record @m. */
> @@ -632,6 +635,7 @@ err_out:
>   "after umounting to correct this.", -err);
>   NVolSetErrors(vol);
>   }
> + kfree(bhs);
>   return err;
> }
> 
> @@ -673,7 +677,7 @@ int write_mft_record_nolock(ntfs_inode *ni, MFT_RECORD 
> *m, int sync)
>   unsigned int blocksize = vol->sb->s_blocksize;
>   unsigned char blocksize_bits = vol->sb->s_blocksize_bits;
>   int max_bhs = vol->mft_record_size / blocksize;
> - struct buffer_head *bhs[max_bhs];
> + struct buffer_head **bhs;
>   struct buffer_head *bh, *head;
>   runlist_element *rl;
>   unsigned int block_start, block_end, m_start, m_end;
> @@ -689,7 +693,8 @@ int write_mft_record_nolock(ntfs_inode *ni, MFT_RECORD 
> *m, int sync)
>* There is no danger of races since the caller is holding the locks
>* for the mft record @m and the page it is in.
>*/
> - if (!NInoTestClearDirty(ni))
> + bhs = kmalloc(max_bhs * sizeof(struct buffer_head *), GFP_NOFS);
> + if (!bhs || !NInoTestClearDirty(ni))
>   goto done;
>   bh = head = page_buffers(page);
>   BUG_ON(!bh);
> @@ -820,6 +825,7 @@ int write_mft_record_nolock(ntfs_inode *ni, MFT_RECORD 
> *m, int sync)
>   goto err_out;
>   }
> done:
> + kfree(bhs);
>   ntfs_debug("Done.");
>   return 0;
> cleanup_out:
> @@ -840,6 +846,7 @@ err_out:
>   err = 0;
>   } else
>   NVolSetErrors(vol);
> + kfree(bhs);
>   return err;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] fs/ntfs/mft.c: fix 2 sparse warnings

2014-04-28 Thread Anton Altaparmakov

NAK.

I do not see why this is needed.  Not all sparse warnings need fixing.

What is so bad about a variable length array that we need to incur kmalloc 
overhead on each call?  I would have thought the CPU overhead of the kmalloc is 
a lot larger than any CPU overhead of a variable length array...

The array size is tiny - basically on all volumes mft_record_size is 1024 and 
blocksize is either 512 or 1024 thus the array size is either 1 or 2 pointers.  
The reason it is dynamic is just in case Microsoft change their mind and 
increase the mft_record_size in a future NTFS/Windows version to something 
bigger than 1024 bytes, the current code would still cope fine.

Note mft_record_size is the on disk inode size thus it will never be much 
bigger and unlikely to ever change from the 1024 really.  A very long time ago 
when NTFS was being developed Microsoft used 2048 bytes but quickly changed it 
to 1024 as most inodes do not need anywhere near that much space for an inode.

Best regards,

Anton

On 27 Apr 2014, at 11:12, Fabian Frederick f...@skynet.be wrote:

 fs/ntfs/mft.c:471:33: warning: Variable length array is used.
 fs/ntfs/mft.c:676:33: warning: Variable length array is used.
 
 This is untested.
 
 Cc: Anton Altaparmakov an...@tuxera.com
 Cc: Andrew Morton a...@linux-foundation.org
 Signed-off-by: Fabian Frederick f...@skynet.be
 ---
 fs/ntfs/mft.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)
 
 diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
 index 3014a36..eddb739 100644
 --- a/fs/ntfs/mft.c
 +++ b/fs/ntfs/mft.c
 @@ -468,7 +468,7 @@ int ntfs_sync_mft_mirror(ntfs_volume *vol, const unsigned 
 long mft_no,
   struct page *page;
   unsigned int blocksize = vol-sb-s_blocksize;
   int max_bhs = vol-mft_record_size / blocksize;
 - struct buffer_head *bhs[max_bhs];
 + struct buffer_head **bhs;
   struct buffer_head *bh, *head;
   u8 *kmirr;
   runlist_element *rl;
 @@ -478,11 +478,14 @@ int ntfs_sync_mft_mirror(ntfs_volume *vol, const 
 unsigned long mft_no,
 
   ntfs_debug(Entering for inode 0x%lx., mft_no);
   BUG_ON(!max_bhs);
 - if (unlikely(!vol-mftmirr_ino)) {
 + bhs = kmalloc(max_bhs * sizeof(struct buffer_head *), GFP_NOFS);
 + if (unlikely(!bhs || !vol-mftmirr_ino)) {
   /* This could happen during umount... */
   err = ntfs_sync_mft_mirror_umount(vol, mft_no, m);
 - if (likely(!err))
 + if (likely(!err)) {
 + kfree(bhs);
   return err;
 + }
   goto err_out;
   }
   /* Get the page containing the mirror copy of the mft record @m. */
 @@ -632,6 +635,7 @@ err_out:
   after umounting to correct this., -err);
   NVolSetErrors(vol);
   }
 + kfree(bhs);
   return err;
 }
 
 @@ -673,7 +677,7 @@ int write_mft_record_nolock(ntfs_inode *ni, MFT_RECORD 
 *m, int sync)
   unsigned int blocksize = vol-sb-s_blocksize;
   unsigned char blocksize_bits = vol-sb-s_blocksize_bits;
   int max_bhs = vol-mft_record_size / blocksize;
 - struct buffer_head *bhs[max_bhs];
 + struct buffer_head **bhs;
   struct buffer_head *bh, *head;
   runlist_element *rl;
   unsigned int block_start, block_end, m_start, m_end;
 @@ -689,7 +693,8 @@ int write_mft_record_nolock(ntfs_inode *ni, MFT_RECORD 
 *m, int sync)
* There is no danger of races since the caller is holding the locks
* for the mft record @m and the page it is in.
*/
 - if (!NInoTestClearDirty(ni))
 + bhs = kmalloc(max_bhs * sizeof(struct buffer_head *), GFP_NOFS);
 + if (!bhs || !NInoTestClearDirty(ni))
   goto done;
   bh = head = page_buffers(page);
   BUG_ON(!bh);
 @@ -820,6 +825,7 @@ int write_mft_record_nolock(ntfs_inode *ni, MFT_RECORD 
 *m, int sync)
   goto err_out;
   }
 done:
 + kfree(bhs);
   ntfs_debug(Done.);
   return 0;
 cleanup_out:
 @@ -840,6 +846,7 @@ err_out:
   err = 0;
   } else
   NVolSetErrors(vol);
 + kfree(bhs);
   return err;
 }
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] NTFS: Remove NULL value assignments

Hi Andrew,

Please could you merge this via your patch series?  Thanks a lot in advance!

Best regards,

Anton

On 9 Apr 2014, at 18:01, Fabian Frederick  wrote:

> Static values are automatically initialized to NULL.
> 
> Acked-by: Anton Altaparmakov 
> Cc: Andrew Morton 
> Cc: Anton Altaparmakov 
> Signed-off-by: Fabian Frederick 
> ---
> fs/ntfs/compress.c | 2 +-
> fs/ntfs/super.c| 4 ++--
> fs/ntfs/sysctl.c   | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ntfs/compress.c b/fs/ntfs/compress.c
> index ee4144c..f82498c 100644
> --- a/fs/ntfs/compress.c
> +++ b/fs/ntfs/compress.c
> @@ -58,7 +58,7 @@ typedef enum {
> /**
>  * ntfs_compression_buffer - one buffer for the decompression engine
>  */
> -static u8 *ntfs_compression_buffer = NULL;
> +static u8 *ntfs_compression_buffer;
> 
> /**
>  * ntfs_cb_lock - spinlock which protects ntfs_compression_buffer
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index 9de2491..6c3296e 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -50,8 +50,8 @@
> static unsigned long ntfs_nr_compression_users;
> 
> /* A global default upcase table and a corresponding reference count. */
> -static ntfschar *default_upcase = NULL;
> -static unsigned long ntfs_nr_upcase_users = 0;
> +static ntfschar *default_upcase;
> +static unsigned long ntfs_nr_upcase_users;
> 
> /* Error constants/strings used in inode.c::ntfs_show_options(). */
> typedef enum {
> diff --git a/fs/ntfs/sysctl.c b/fs/ntfs/sysctl.c
> index 79a8918..1927170 100644
> --- a/fs/ntfs/sysctl.c
> +++ b/fs/ntfs/sysctl.c
> @@ -56,7 +56,7 @@ static ctl_table sysctls_root[] = {
> };
> 
> /* Storage for the sysctls header. */
> -static struct ctl_table_header *sysctls_root_table = NULL;
> +static struct ctl_table_header *sysctls_root_table;
> 
> /**
>  * ntfs_sysctl - add or remove the debug sysctl

-- 
Anton Altaparmakov  (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] NTFS: Remove NULL value assignments

Hi Fabian,

Thanks.  Looks good, but why leave the "static ... = 0;" cases if you are doing 
the NULL ones?

Do you feel like redoing it to cover the = 0 cases as well?

Anyway, you can add my Acked-By line.

Best regards,

Anton

On 8 Apr 2014, at 21:12, Fabian Frederick  wrote:

> Static values are automatically initialized to NULL.
> 
> Cc: Andrew Morton 
> Cc: Anton Altaparmakov 
> Signed-off-by: Fabian Frederick 
> ---
> fs/ntfs/compress.c | 2 +-
> fs/ntfs/super.c| 2 +-
> fs/ntfs/sysctl.c   | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ntfs/compress.c b/fs/ntfs/compress.c
> index ee4144c..f82498c 100644
> --- a/fs/ntfs/compress.c
> +++ b/fs/ntfs/compress.c
> @@ -58,7 +58,7 @@ typedef enum {
> /**
>  * ntfs_compression_buffer - one buffer for the decompression engine
>  */
> -static u8 *ntfs_compression_buffer = NULL;
> +static u8 *ntfs_compression_buffer;
> 
> /**
>  * ntfs_cb_lock - spinlock which protects ntfs_compression_buffer
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index 9de2491..eed5119 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -50,7 +50,7 @@
> static unsigned long ntfs_nr_compression_users;
> 
> /* A global default upcase table and a corresponding reference count. */
> -static ntfschar *default_upcase = NULL;
> +static ntfschar *default_upcase;
> static unsigned long ntfs_nr_upcase_users = 0;
> 
> /* Error constants/strings used in inode.c::ntfs_show_options(). */
> diff --git a/fs/ntfs/sysctl.c b/fs/ntfs/sysctl.c
> index 79a8918..1927170 100644
> --- a/fs/ntfs/sysctl.c
> +++ b/fs/ntfs/sysctl.c
> @@ -56,7 +56,7 @@ static ctl_table sysctls_root[] = {
> };
> 
> /* Storage for the sysctls header. */
> -static struct ctl_table_header *sysctls_root_table = NULL;
> +static struct ctl_table_header *sysctls_root_table;
> 
> /**
>  * ntfs_sysctl - add or remove the debug sysctl
> -- 
> 1.8.4.5

-- 
Anton Altaparmakov  (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] NTFS: Remove NULL value assignments

Hi Fabian,

Thanks.  Looks good, but why leave the static ... = 0; cases if you are doing 
the NULL ones?

Do you feel like redoing it to cover the = 0 cases as well?

Anyway, you can add my Acked-By line.

Best regards,

Anton

On 8 Apr 2014, at 21:12, Fabian Frederick f...@skynet.be wrote:

 Static values are automatically initialized to NULL.
 
 Cc: Andrew Morton a...@linux-foundation.org
 Cc: Anton Altaparmakov an...@tuxera.com
 Signed-off-by: Fabian Frederick f...@skynet.be
 ---
 fs/ntfs/compress.c | 2 +-
 fs/ntfs/super.c| 2 +-
 fs/ntfs/sysctl.c   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
 
 diff --git a/fs/ntfs/compress.c b/fs/ntfs/compress.c
 index ee4144c..f82498c 100644
 --- a/fs/ntfs/compress.c
 +++ b/fs/ntfs/compress.c
 @@ -58,7 +58,7 @@ typedef enum {
 /**
  * ntfs_compression_buffer - one buffer for the decompression engine
  */
 -static u8 *ntfs_compression_buffer = NULL;
 +static u8 *ntfs_compression_buffer;
 
 /**
  * ntfs_cb_lock - spinlock which protects ntfs_compression_buffer
 diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
 index 9de2491..eed5119 100644
 --- a/fs/ntfs/super.c
 +++ b/fs/ntfs/super.c
 @@ -50,7 +50,7 @@
 static unsigned long ntfs_nr_compression_users;
 
 /* A global default upcase table and a corresponding reference count. */
 -static ntfschar *default_upcase = NULL;
 +static ntfschar *default_upcase;
 static unsigned long ntfs_nr_upcase_users = 0;
 
 /* Error constants/strings used in inode.c::ntfs_show_options(). */
 diff --git a/fs/ntfs/sysctl.c b/fs/ntfs/sysctl.c
 index 79a8918..1927170 100644
 --- a/fs/ntfs/sysctl.c
 +++ b/fs/ntfs/sysctl.c
 @@ -56,7 +56,7 @@ static ctl_table sysctls_root[] = {
 };
 
 /* Storage for the sysctls header. */
 -static struct ctl_table_header *sysctls_root_table = NULL;
 +static struct ctl_table_header *sysctls_root_table;
 
 /**
  * ntfs_sysctl - add or remove the debug sysctl
 -- 
 1.8.4.5

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH v2] NTFS: Remove NULL value assignments

Hi Andrew,

Please could you merge this via your patch series?  Thanks a lot in advance!

Best regards,

Anton

On 9 Apr 2014, at 18:01, Fabian Frederick f...@skynet.be wrote:

 Static values are automatically initialized to NULL.
 
 Acked-by: Anton Altaparmakov an...@tuxera.com
 Cc: Andrew Morton a...@linux-foundation.org
 Cc: Anton Altaparmakov an...@tuxera.com
 Signed-off-by: Fabian Frederick f...@skynet.be
 ---
 fs/ntfs/compress.c | 2 +-
 fs/ntfs/super.c| 4 ++--
 fs/ntfs/sysctl.c   | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
 
 diff --git a/fs/ntfs/compress.c b/fs/ntfs/compress.c
 index ee4144c..f82498c 100644
 --- a/fs/ntfs/compress.c
 +++ b/fs/ntfs/compress.c
 @@ -58,7 +58,7 @@ typedef enum {
 /**
  * ntfs_compression_buffer - one buffer for the decompression engine
  */
 -static u8 *ntfs_compression_buffer = NULL;
 +static u8 *ntfs_compression_buffer;
 
 /**
  * ntfs_cb_lock - spinlock which protects ntfs_compression_buffer
 diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
 index 9de2491..6c3296e 100644
 --- a/fs/ntfs/super.c
 +++ b/fs/ntfs/super.c
 @@ -50,8 +50,8 @@
 static unsigned long ntfs_nr_compression_users;
 
 /* A global default upcase table and a corresponding reference count. */
 -static ntfschar *default_upcase = NULL;
 -static unsigned long ntfs_nr_upcase_users = 0;
 +static ntfschar *default_upcase;
 +static unsigned long ntfs_nr_upcase_users;
 
 /* Error constants/strings used in inode.c::ntfs_show_options(). */
 typedef enum {
 diff --git a/fs/ntfs/sysctl.c b/fs/ntfs/sysctl.c
 index 79a8918..1927170 100644
 --- a/fs/ntfs/sysctl.c
 +++ b/fs/ntfs/sysctl.c
 @@ -56,7 +56,7 @@ static ctl_table sysctls_root[] = {
 };
 
 /* Storage for the sysctls header. */
 -static struct ctl_table_header *sysctls_root_table = NULL;
 +static struct ctl_table_header *sysctls_root_table;
 
 /**
  * ntfs_sysctl - add or remove the debug sysctl

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH 1/1] NTFS: Logging clean-up

Hi,

On 16 Mar 2014, at 16:12, Joe Perches  wrote:

> On Sun, 2014-03-16 at 15:53 +0000, Anton Altaparmakov wrote:
>> Hi,
>> 
>> Looks good, thanks.  You can add my Acked-by if you like.  Can I assume you 
>> have test it builds?
>> 
>> Andrew, can you please merge this via your patch series?  Thanks!
> 
> I think a few things need fixing first

Yes, quite right - I had assume that had been tested!

> On 16 Mar 2014, at 12:27, Fabian Frederick  wrote:
>> 
>>> -All printk(KERN_foo converted to pr_foo().
>>> -Add pr_fmt and remove redundant prefixes.
> 
> I'm not sure there are "redundant prefixes"
> 
> This changes prefixes from "NTFS-fs" to "ntfs"
> and should be at a minimum mentioned in the
> changelog.

As long as it says ntfs in the string so dmesg output can be "grep -i ntfs"-ed 
I am not fussed.

> The va_end location needs moving.

Yes, well spotted, thanks.

> Converting printk(KERN_DEBUG -> pr_debug will
> not always emit that message now.  Now, only if
> DEBUG is #defined or dynamic_debugging is enabled
> for the build _and_ the message is specifically
> enabled will the message be output.
> 
> So, the debugging output has been silenced by default.
> 
> That's not necessarily good.

No that is not good at all.  I didn't know about those pr_debug semantics so 
thank you for pointing them out.  That needs fixing otherwise we might as well 
not have the messages at all...  Being able to enable all ntfs debug messages 
using a insmod option or via /proc as is at the moment is a very valuable 
debugging too and I have scripts that use this so am not keen on this being 
changed at all.

Fabian, can you please fix the issues pointed out by Joe and also please make 
sure you actually test it!

Thanks a lot in advance!

Best regards,

Anton

> diff --git a/fs/ntfs/debug.c b/fs/ntfs/debug.c
> []
>>> @@ -59,17 +53,15 @@ void __ntfs_warning(const char *function, const struct 
>>> super_block *sb,
>>> #endif
>>> if (function)
>>> flen = strlen(function);
>>> -   spin_lock(_buf_lock);
>>> va_start(args, fmt);
>>> -   vsnprintf(err_buf, sizeof(err_buf), fmt, args);
>>> +   vaf.fmt = fmt;
>>> +   vaf.va = 
> 
>>> va_end(args);
> 
> This va_end should be moved after the pr_warns.
> 
>>> if (sb)
>>> -   printk(KERN_ERR "NTFS-fs warning (device %s): %s(): %s\n",
>>> -   sb->s_id, flen ? function : "", err_buf);
>>> +   pr_warn("(device %s): %s(): %pV\n",
>>> +   sb->s_id, flen ? function : "", );
>>> else
>>> -   printk(KERN_ERR "NTFS-fs warning: %s(): %s\n",
>>> -   flen ? function : "", err_buf);
>>> -   spin_unlock(_buf_lock);
>>> +   pr_warn("%s(): %pV\n", flen ? function : "", );
>>> }
>>> 
>>> /**
>>> @@ -94,6 +86,7 @@ void __ntfs_warning(const char *function, const struct 
>>> super_block *sb,
>>> void __ntfs_error(const char *function, const struct super_block *sb,
>>> const char *fmt, ...)
>>> {
>>> +   struct va_format vaf;
>>> va_list args;
>>> int flen = 0;
>>> 
>>> @@ -103,17 +96,15 @@ void __ntfs_error(const char *function, const struct 
>>> super_block *sb,
>>> #endif
>>> if (function)
>>> flen = strlen(function);
>>> -   spin_lock(_buf_lock);
>>> va_start(args, fmt);
>>> -   vsnprintf(err_buf, sizeof(err_buf), fmt, args);
>>> +   vaf.fmt = fmt;
>>> +   vaf.va = 
>>> va_end(args);
> 
> Here too
> 
>>> if (sb)
>>> -   printk(KERN_ERR "NTFS-fs error (device %s): %s(): %s\n",
>>> -   sb->s_id, flen ? function : "", err_buf);
>>> +   pr_err("(device %s): %s(): %pV\n",
>>> +  sb->s_id, flen ? function : "", );
>>> else
>>> -   printk(KERN_ERR "NTFS-fs error: %s(): %s\n",
>>> -   flen ? function : "", err_buf);
>>> -   spin_unlock(_buf_lock);
>>> +   pr_err("%s(): %pV\n", flen ? function : "", );


-- 
Anton Altaparmakov  (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] NTFS: Logging clean-up

to name_err_out;
>   }
> 
> @@ -3139,8 +3138,7 @@ static int __init init_ntfs_fs(void)
>   sizeof(ntfs_inode), 0,
>   SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD, NULL);
>   if (!ntfs_inode_cache) {
> - printk(KERN_CRIT "NTFS: Failed to create %s!\n",
> - ntfs_inode_cache_name);
> + pr_crit("Failed to create %s!\n", ntfs_inode_cache_name);
>   goto inode_err_out;
>   }
> 
> @@ -3149,15 +3147,14 @@ static int __init init_ntfs_fs(void)
>   SLAB_HWCACHE_ALIGN|SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD,
>   ntfs_big_inode_init_once);
>   if (!ntfs_big_inode_cache) {
> - printk(KERN_CRIT "NTFS: Failed to create %s!\n",
> - ntfs_big_inode_cache_name);
> + pr_crit("Failed to create %s!\n", ntfs_big_inode_cache_name);
>   goto big_inode_err_out;
>   }
> 
>   /* Register the ntfs sysctls. */
>   err = ntfs_sysctl(1);
>   if (err) {
> - printk(KERN_CRIT "NTFS: Failed to register NTFS sysctls!\n");
> + pr_crit("Failed to register NTFS sysctls!\n");
>   goto sysctl_err_out;
>   }
> 
> @@ -3166,7 +3163,7 @@ static int __init init_ntfs_fs(void)
>   ntfs_debug("NTFS driver registered successfully.");
>   return 0; /* Success! */
>   }
> - printk(KERN_CRIT "NTFS: Failed to register NTFS filesystem driver!\n");
> + pr_crit("Failed to register NTFS filesystem driver!\n");
> 
>   /* Unregister the ntfs sysctls. */
>   ntfs_sysctl(0);
> @@ -3182,8 +3179,7 @@ actx_err_out:
>   kmem_cache_destroy(ntfs_index_ctx_cache);
> ictx_err_out:
>   if (!err) {
> - printk(KERN_CRIT "NTFS: Aborting NTFS filesystem driver "
> - "registration...\n");
> + pr_crit("Aborting NTFS filesystem driver registration...\n");
>   err = -ENOMEM;
>   }
>   return err;
-- 
Anton Altaparmakov  (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] NTFS: Logging clean-up

 to register NTFS filesystem driver!\n);
 + pr_crit(Failed to register NTFS filesystem driver!\n);
 
   /* Unregister the ntfs sysctls. */
   ntfs_sysctl(0);
 @@ -3182,8 +3179,7 @@ actx_err_out:
   kmem_cache_destroy(ntfs_index_ctx_cache);
 ictx_err_out:
   if (!err) {
 - printk(KERN_CRIT NTFS: Aborting NTFS filesystem driver 
 - registration...\n);
 + pr_crit(Aborting NTFS filesystem driver registration...\n);
   err = -ENOMEM;
   }
   return err;
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH 1/1] NTFS: Logging clean-up

Hi,

On 16 Mar 2014, at 16:12, Joe Perches j...@perches.com wrote:

 On Sun, 2014-03-16 at 15:53 +, Anton Altaparmakov wrote:
 Hi,
 
 Looks good, thanks.  You can add my Acked-by if you like.  Can I assume you 
 have test it builds?
 
 Andrew, can you please merge this via your patch series?  Thanks!
 
 I think a few things need fixing first

Yes, quite right - I had assume that had been tested!

 On 16 Mar 2014, at 12:27, Fabian Frederick f...@skynet.be wrote:
 
 -All printk(KERN_foo converted to pr_foo().
 -Add pr_fmt and remove redundant prefixes.
 
 I'm not sure there are redundant prefixes
 
 This changes prefixes from NTFS-fs to ntfs
 and should be at a minimum mentioned in the
 changelog.

As long as it says ntfs in the string so dmesg output can be grep -i ntfs-ed 
I am not fussed.

 The va_end location needs moving.

Yes, well spotted, thanks.

 Converting printk(KERN_DEBUG - pr_debug will
 not always emit that message now.  Now, only if
 DEBUG is #defined or dynamic_debugging is enabled
 for the build _and_ the message is specifically
 enabled will the message be output.
 
 So, the debugging output has been silenced by default.
 
 That's not necessarily good.

No that is not good at all.  I didn't know about those pr_debug semantics so 
thank you for pointing them out.  That needs fixing otherwise we might as well 
not have the messages at all...  Being able to enable all ntfs debug messages 
using a insmod option or via /proc as is at the moment is a very valuable 
debugging too and I have scripts that use this so am not keen on this being 
changed at all.

Fabian, can you please fix the issues pointed out by Joe and also please make 
sure you actually test it!

Thanks a lot in advance!

Best regards,

Anton

 diff --git a/fs/ntfs/debug.c b/fs/ntfs/debug.c
 []
 @@ -59,17 +53,15 @@ void __ntfs_warning(const char *function, const struct 
 super_block *sb,
 #endif
 if (function)
 flen = strlen(function);
 -   spin_lock(err_buf_lock);
 va_start(args, fmt);
 -   vsnprintf(err_buf, sizeof(err_buf), fmt, args);
 +   vaf.fmt = fmt;
 +   vaf.va = args;
 
 va_end(args);
 
 This va_end should be moved after the pr_warns.
 
 if (sb)
 -   printk(KERN_ERR NTFS-fs warning (device %s): %s(): %s\n,
 -   sb-s_id, flen ? function : , err_buf);
 +   pr_warn((device %s): %s(): %pV\n,
 +   sb-s_id, flen ? function : , vaf);
 else
 -   printk(KERN_ERR NTFS-fs warning: %s(): %s\n,
 -   flen ? function : , err_buf);
 -   spin_unlock(err_buf_lock);
 +   pr_warn(%s(): %pV\n, flen ? function : , vaf);
 }
 
 /**
 @@ -94,6 +86,7 @@ void __ntfs_warning(const char *function, const struct 
 super_block *sb,
 void __ntfs_error(const char *function, const struct super_block *sb,
 const char *fmt, ...)
 {
 +   struct va_format vaf;
 va_list args;
 int flen = 0;
 
 @@ -103,17 +96,15 @@ void __ntfs_error(const char *function, const struct 
 super_block *sb,
 #endif
 if (function)
 flen = strlen(function);
 -   spin_lock(err_buf_lock);
 va_start(args, fmt);
 -   vsnprintf(err_buf, sizeof(err_buf), fmt, args);
 +   vaf.fmt = fmt;
 +   vaf.va = args;
 va_end(args);
 
 Here too
 
 if (sb)
 -   printk(KERN_ERR NTFS-fs error (device %s): %s(): %s\n,
 -   sb-s_id, flen ? function : , err_buf);
 +   pr_err((device %s): %s(): %pV\n,
 +  sb-s_id, flen ? function : , vaf);
 else
 -   printk(KERN_ERR NTFS-fs error: %s(): %s\n,
 -   flen ? function : , err_buf);
 -   spin_unlock(err_buf_lock);
 +   pr_err(%s(): %pV\n, flen ? function : , vaf);


-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH 1/1] Documentation/filesystems/ntfs.txt: Remove changelog reference

2014-03-02 Thread Anton Altaparmakov

Looks good, thanks.

Andrew, if you haven't already, can you please take that one for integration 
via your mm series?  Thanks!

Best regards,

Anton

On 27 Feb 2014, at 11:43, Fabian Frederick  wrote:

> File was removed in the following commit :
> 7c821a179f91c3ad52588400ce52a7fb48b9868c ("Remove fs/ntfs/ChangeLog")
> 
> Signed-off-by: Fabian Frederick 
> ---
> Documentation/filesystems/ntfs.txt | 2 --
> 1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/filesystems/ntfs.txt 
> b/Documentation/filesystems/ntfs.txt
> index 791af8d..61947fa 100644
> --- a/Documentation/filesystems/ntfs.txt
> +++ b/Documentation/filesystems/ntfs.txt
> @@ -455,8 +455,6 @@ not have this problem with odd numbers of sectors.
> ChangeLog
> =
> 
> -Note, a technical ChangeLog aimed at kernel hackers is in fs/ntfs/ChangeLog.
> -
> 2.1.30:
>   - Fix writev() (it kept writing the first segment over and over again
> instead of moving onto subsequent segments).
> -- 
> 1.8.1.4

-- 
Anton Altaparmakov  (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] Documentation/filesystems/ntfs.txt: Remove changelog reference

2014-03-02 Thread Anton Altaparmakov

Looks good, thanks.

Andrew, if you haven't already, can you please take that one for integration 
via your mm series?  Thanks!

Best regards,

Anton

On 27 Feb 2014, at 11:43, Fabian Frederick f...@skynet.be wrote:

 File was removed in the following commit :
 7c821a179f91c3ad52588400ce52a7fb48b9868c (Remove fs/ntfs/ChangeLog)
 
 Signed-off-by: Fabian Frederick f...@skynet.be
 ---
 Documentation/filesystems/ntfs.txt | 2 --
 1 file changed, 2 deletions(-)
 
 diff --git a/Documentation/filesystems/ntfs.txt 
 b/Documentation/filesystems/ntfs.txt
 index 791af8d..61947fa 100644
 --- a/Documentation/filesystems/ntfs.txt
 +++ b/Documentation/filesystems/ntfs.txt
 @@ -455,8 +455,6 @@ not have this problem with odd numbers of sectors.
 ChangeLog
 =
 
 -Note, a technical ChangeLog aimed at kernel hackers is in fs/ntfs/ChangeLog.
 -
 2.1.30:
   - Fix writev() (it kept writing the first segment over and over again
 instead of moving onto subsequent segments).
 -- 
 1.8.1.4

-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH] hfsplus: Remove hfsplus_file_lookup

2013-12-11 Thread Anton Altaparmakov

Hi,

On 11 Dec 2013, at 19:11, Al Viro  wrote:
> On Wed, Dec 11, 2013 at 10:49:29PM +0300, Vyacheslav Dubeyko wrote:
>> This feature worked earlier under Linux. So, I suppose that some changes in 
>> HFS+ driver
>> or in VFS broke it. And it needs to investigate and fix the reported issue. 
>> Thank you for the
>> report.
> 
> This "feature" is severely broken and yes, outright removal is what I'd
> suggest for a fix.  HFS+ allows hardlinks to files, which means that
> you allow multiple dentries for the same inode with ->lookup() in it,
> which is asking for deadlocks.
> 
> This is fundamentally not supported.  Considering that forks are lousy
> idea in the first place, I'd seriously suggest to remove that idiocy for
> good.

Completely agree with Al.  If anyone really wants access to forks they can 
implement them via the xattr interface (ok it has the 64k limitation but most 
forks are quite small so not much of an issue).  That's how I implemented 
access to named streams in Tuxera NTFS and it works a treat (and allows Linux 
apps and various security modules that require xattr support to work properly 
which is also great).

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] hfsplus: Remove hfsplus_file_lookup

2013-12-11 Thread Anton Altaparmakov

Hi,

On 11 Dec 2013, at 19:11, Al Viro v...@zeniv.linux.org.uk wrote:
 On Wed, Dec 11, 2013 at 10:49:29PM +0300, Vyacheslav Dubeyko wrote:
 This feature worked earlier under Linux. So, I suppose that some changes in 
 HFS+ driver
 or in VFS broke it. And it needs to investigate and fix the reported issue. 
 Thank you for the
 report.
 
 This feature is severely broken and yes, outright removal is what I'd
 suggest for a fix.  HFS+ allows hardlinks to files, which means that
 you allow multiple dentries for the same inode with -lookup() in it,
 which is asking for deadlocks.
 
 This is fundamentally not supported.  Considering that forks are lousy
 idea in the first place, I'd seriously suggest to remove that idiocy for
 good.

Completely agree with Al.  If anyone really wants access to forks they can 
implement them via the xattr interface (ok it has the 64k limitation but most 
forks are quite small so not much of an issue).  That's how I implemented 
access to named streams in Tuxera NTFS and it works a treat (and allows Linux 
apps and various security modules that require xattr support to work properly 
which is also great).

Best regards,

Anton
-- 
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH] add exFAT driver

Hi,

On 25 Sep 2013, at 23:29, Alexander Holler  wrote:
> Am 26.09.2013 00:10, schrieb Greg Kroah-Hartman:
>> Please stick to technical discussions about the code on the kernel
>> mailing lists.  Legal discussions can be left up to the lawyers, of
>> which we are not.
> 
> Hmm, but I would like to know if someone has to fear getting owned by
> Microsoft if he would use that driver.
> 
> Giving the rumours about Linux companies having to pay Microsoft and
> giving the fact that all of those licencees seem to don't have to speak
> about what Microsoft claims patents for and for what they have to pay, I
> obviously think adding that driver to Linux and thus making exFAT more
> general accepted is a very bad idea.
> 
> Of course, I'm not a lawyer too, but as a responsible Linux developer, I
> should at least be able to warn other parities when they approach me and
> want to use exFAT. Doing such without the maybe necessary license might
> drive small companies into the ground because most of them are unable to
> even think about having the money needed to talk with Microsoft lawyers
> in front of a court.


Exactly.  That is all I was trying to do.  Warn people/companies not to use the 
driver because they may get sued for using it.  As the below Microsoft exFAT 
licensing page says at the bottom:


Please note that open source or other publicly available implementations of 
exFAT do not include an IP license from Microsoft. For licensing information, 
please contact iplic...@microsoft.com.


Above is from bottom of:


http://www.microsoft.com/en-us/legal/intellectualproperty/IPLicensing/Programs/exFATFileSystem.aspx

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add exFAT driver

Hi,

On 25 Sep 2013, at 23:10, Greg Kroah-Hartman  wrote:
> On Wed, Sep 25, 2013 at 10:44:15PM +0100, Anton Altaparmakov wrote:
>> On 25 Sep 2013, at 21:21, Greg Kroah-Hartman  
>> wrote:
>>> On Wed, Sep 25, 2013 at 09:28:56PM +0200, Alexander Holler wrote:
>>>> 
>>>> Maybe a silly question, but isn't exFAT protected by some MS owned
>>>> patents which might drive Linux users into the hand of MS lawyers as
>>>> already happened with FAT?
>> 
>> Yes, it is.  You cannot use exFAT without a Microsoft patent license
>> (unless you live in countries without software patents perhaps).
> 
> Given that you that you are not a Microsoft representative, nor a
> Samsung employee, I don't understand how you can make such a definitive
> statement.

Have you actually read the source code that was released?

May I quote just one bit:

from exfat_1.2.4/exfat.c (available from http://opensource.samsung.com - just 
search for exfat) at the top:

/* Some of the source code in this file came from "linux/fs/fat/misc.c".  */
/*
 *  linux/fs/fat/misc.c
 *
 *  Written 1992,1993 by Werner Almesberger
 *  22/11/2000 - Fixed fat_date_unix2dos for dates earlier than 01/01/1980
 * and date_dos2unix for date==0 by Igor Zhbanov(b...@uniyar.ac.ru)
 */

That is somewhat conclusively a derivative work is it not?

Also, have a read of this article:

http://www.phoronix.com/scan.php?page=news_item=MTQzODQ

Which explains further who made them open source it after they saw the leaked 
code on github.

So even without resorting to knowledge I may not discuss it is pretty 
conclusive that what I said is correct as anyone driving google can find for 
themselves as I pointed out above...

>>>> It would make me wonder if not. Maybe you could ask Samsung about
>>>> that too, when you are there.
>>> 
>>> Because Samsung released the code under the GPLv2, and their lawyers
>>> understand what that means, should answer any question you might have
>>> about this.
>> 
>> Sorry but you have no idea what you are talking about.
> 
> Ah, that's a lovely way to engage in a conversation.

I did say sorry!  (-;

>> Samsung modified the GPL-ed FAT driver to make it work with exFAT.
>> Therefore their exFAT driver was GPL as a derived work.  They got
>> caught and had to release the source code.
> 
> And now you claim to be a Samsung representative again, I think your
> country has some bad liable laws you might wish to watch out for...

I am not claiming anything and least of all to be representing Samsung!!!

Libel implies untruth and as you can see above I am only stating what anyone 
can readily find on google.

> This isn't going to go very far, so I'll just not respond anymore, it's
> not going to be productive, and given that I don't see your name on the
> code here, I don't see why I need to.
> 
> Please stick to technical discussions about the code on the kernel
> mailing lists.  Legal discussions can be left up to the lawyers, of
> which we are not.

I agree, but then please stop making public assertions that people can use the 
exfat driver legally.  You just yourself said you are not a lawyer so I do not 
understand how you can make your assertion!

If anyone cares, here is Microsoft's exFAT licensing page which I strongly 
recommend you read before you use that driver:

http://www.microsoft.com/en-us/legal/intellectualproperty/IPLicensing/Programs/exFATFileSystem.aspx

Also if you search google for "exFAT patent" you can find some that way but 
there are also others that are not found that way but that are clearly 
essential for any exFAT implementation (according to the technical review I did 
of them).  I am not sure whether I am allowed to give a list or not so I will 
refrain from doing so.

Best regards,

Anton
-- 
Anton Altaparmakov  (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add exFAT driver

Hi,

On 25 Sep 2013, at 23:10, Greg Kroah-Hartman gre...@linuxfoundation.org wrote:
On Wed, Sep 25, 2013 at 10:44:15PM +0100, Anton Altaparmakov wrote:
On 25 Sep 2013, at 21:21, Greg Kroah-Hartman gre...@linuxfoundation.org
wrote:
On Wed, Sep 25, 2013 at 09:28:56PM +0200, Alexander Holler wrote:

Maybe a silly question, but isn't exFAT protected by some MS owned
patents which might drive Linux users into the hand of MS lawyers as
already happened with FAT?

Yes, it is. You cannot use exFAT without a Microsoft patent license
(unless you live in countries without software patents perhaps).

Given that you that you are not a Microsoft representative, nor a
Samsung employee, I don't understand how you can make such a definitive
statement.

Have you actually read the source code that was released?

May I quote just one bit:

from exfat_1.2.4/exfat.c (available from http://opensource.samsung.com - just
search for exfat) at the top:

/* Some of the source code in this file came from linux/fs/fat/misc.c. */
/*
* linux/fs/fat/misc.c
*
* Written 1992,1993 by Werner Almesberger
* 22/11/2000 - Fixed fat_date_unix2dos for dates earlier than 01/01/1980
* and date_dos2unix for date==0 by Igor Zhbanov(b...@uniyar.ac.ru)
*/

That is somewhat conclusively a derivative work is it not?

Also, have a read of this article:

http://www.phoronix.com/scan.php?page=news_itempx=MTQzODQ

Which explains further who made them open source it after they saw the leaked
code on github.

So even without resorting to knowledge I may not discuss it is pretty
conclusive that what I said is correct as anyone driving google can find for
themselves as I pointed out above...

It would make me wonder if not. Maybe you could ask Samsung about
that too, when you are there.

Because Samsung released the code under the GPLv2, and their lawyers
understand what that means, should answer any question you might have
about this.

Sorry but you have no idea what you are talking about.

Ah, that's a lovely way to engage in a conversation.

I did say sorry! (-;

Samsung modified the GPL-ed FAT driver to make it work with exFAT.
Therefore their exFAT driver was GPL as a derived work. They got
caught and had to release the source code.

And now you claim to be a Samsung representative again, I think your
country has some bad liable laws you might wish to watch out for...

I am not claiming anything and least of all to be representing Samsung!!!

Libel implies untruth and as you can see above I am only stating what anyone
can readily find on google.

This isn't going to go very far, so I'll just not respond anymore, it's
not going to be productive, and given that I don't see your name on the
code here, I don't see why I need to.

Please stick to technical discussions about the code on the kernel
mailing lists. Legal discussions can be left up to the lawyers, of
which we are not.

I agree, but then please stop making public assertions that people can use the
exfat driver legally. You just yourself said you are not a lawyer so I do not
understand how you can make your assertion!

If anyone cares, here is Microsoft's exFAT licensing page which I strongly
recommend you read before you use that driver:

http://www.microsoft.com/en-us/legal/intellectualproperty/IPLicensing/Programs/exFATFileSystem.aspx

Also if you search google for exFAT patent you can find some that way but
there are also others that are not found that way but that are clearly
essential for any exFAT implementation (according to the technical review I did
of them). I am not sure whether I am allowed to give a list or not so I will
refrain from doing so.

Best regards,

Anton
--
Anton Altaparmakov aia21 at cam.ac.uk (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

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

Re: [PATCH] add exFAT driver