Re: [PATCH v2] Btrfs: Show a warning message if one of objectid reaches its highest value

2016-03-08 Thread Satoru Takeuchi 

On 2016/03/09 11:32, Naohiro Aota wrote:

2016-03-07 12:05 GMT+09:00 Satoru Takeuchi :

- It's better to show a warning message for the exceptional case
   that one of objectid (in most case, inode number) reaches its
   highest value. Show this message only once to avoid filling
   dmesg with it.
- EOVERFLOW is more proper return value for this case.
   ENOSPC is for "No space left on device" case and objectid isn't
   related to any device.


I have concern about EOVERFLOW. The value returned here will
go through to the user space via btrfs_find_free_ino() and btrfs_create().
It means that "creat" and "mkdir" can now return EOVERFLOW when it
failed to assign new inode number. Such behavior would disagree with
other file systems, which result in user space programs to be
confused.

Also, I don't think EOVERFLOW described in "creat(2)" (or open(2))
suits for this case. As far as I read the following man page from
creat(2),
giving ENOSPC is better option here.


I consider, as I read man, ENOSPC is also doesn't explain
this case. It's not related to pathname.

man 2 creat:

ENOSPC pathname was to be created but the device
containing pathname has no room for the new file.


However, I agree with the ENOSPC is better than EOVERFLOW
because existing code has worked with the former value
in such case.

Next patch will keep the error code as is.

Thank you for your comment, Naota.

Satoru




ENOSPC: pathname was to be created but the device containing pathname has  no  
room  for  the  new file.
EOVERFLOW:
  pathname refers to a regular file that is too large to be opened.
(snip)



Signed-off-by: Satoru Takeuchi 
---
This patch can be applied to 4.5-rc7
---
  fs/btrfs/inode-map.c | 10 +-
  1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode-map.c b/fs/btrfs/inode-map.c
index e50316c..f5e3228 100644
--- a/fs/btrfs/inode-map.c
+++ b/fs/btrfs/inode-map.c
@@ -556,7 +556,15 @@ int btrfs_find_free_objectid(struct btrfs_root *root, u64 
*objectid)
 mutex_lock(&root->objectid_mutex);

 if (unlikely(root->highest_objectid >= BTRFS_LAST_FREE_OBJECTID)) {
-   ret = -ENOSPC;
+   static bool __warned = false;
+
+   if (unlikely(!__warned)) {
+   btrfs_warn(root->fs_info,
+  "The objectid of root %llu reaches its highest 
value.\n",
+  root->root_key.objectid);
+   __warned = true;
+   }
+   ret = -EOVERFLOW;
 goto out;
 }

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

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

Re: [PATCH v2] Btrfs: Show a warning message if one of objectid reaches its highest value

2016-03-08 Thread Satoru Takeuchi 
On 2016/03/09 4:24, Goffredo Baroncelli wrote:
> On 2016-03-07 04:05, Satoru Takeuchi wrote:
>> - It's better to show a warning message for the exceptional case
>> that one of objectid (in most case, inode number) reaches its
>> highest value. Show this message only once to avoid filling
>> dmesg with it.
>> - EOVERFLOW is more proper return value for this case.
>> ENOSPC is for "No space left on device" case and objectid isn't
>> related to any device.
>>
>> Signed-off-by: Satoru Takeuchi 
>> ---
>> This patch can be applied to 4.5-rc7
>> ---
>>fs/btrfs/inode-map.c | 10 +-
>>1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/inode-map.c b/fs/btrfs/inode-map.c
>> index e50316c..f5e3228 100644
>> --- a/fs/btrfs/inode-map.c
>> +++ b/fs/btrfs/inode-map.c
>> @@ -556,7 +556,15 @@ int btrfs_find_free_objectid(struct btrfs_root *root, 
>> u64 *objectid)
>>  mutex_lock(&root->objectid_mutex);
>>
>>  if (unlikely(root->highest_objectid >= BTRFS_LAST_FREE_OBJECTID)) {
>> -ret = -ENOSPC;
>> +static bool __warned = false;
> 
> 
> Please, don't use a static GLOBAL variable. I suggest to move to a "per 
> filesystem" variables for two main reasons:
> 
> 1) if in the (very unlikely) case where two different filesystems reach 
> BTRFS_LAST_FREE_OBJECTID, the first error will hide the second one.
> 
> 2) if you umount and remount the filesystem the error is not shown anymore. A 
> module unload/load or a reboot is required.
> If something strange happens, one of the first thing that the user does, is 
> to umount/remount the filesystem. But the error is not show anymore. This 
> could complicate the diagnosis of the problem.

OK, I see.

I rethink what should this patch be since it's
overkill to prepare per-filesystem variable just
for this warning message.

I'll resend patch which shows this message
every time when hitting this condition instead of
does it just once.

Thanks,
Satoru

> 
> 
> 
> 
>> +
>> +if (unlikely(!__warned)) {
>> +btrfs_warn(root->fs_info,
>> +   "The objectid of root %llu reaches its 
>> highest value.\n",
>> +   root->root_key.objectid);
>> +__warned = true;
>> +}
>> +ret = -EOVERFLOW;
>>  goto out;
>>  }
>>
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Btrfs: Show a warning message if one of objectid reaches its highest value

2016-03-08 Thread Naohiro Aota 
2016-03-07 12:05 GMT+09:00 Satoru Takeuchi :
> - It's better to show a warning message for the exceptional case
>   that one of objectid (in most case, inode number) reaches its
>   highest value. Show this message only once to avoid filling
>   dmesg with it.
> - EOVERFLOW is more proper return value for this case.
>   ENOSPC is for "No space left on device" case and objectid isn't
>   related to any device.

I have concern about EOVERFLOW. The value returned here will
go through to the user space via btrfs_find_free_ino() and btrfs_create().
It means that "creat" and "mkdir" can now return EOVERFLOW when it
failed to assign new inode number. Such behavior would disagree with
other file systems, which result in user space programs to be
confused.

Also, I don't think EOVERFLOW described in "creat(2)" (or open(2))
suits for this case. As far as I read the following man page from
creat(2),
giving ENOSPC is better option here.

> ENOSPC: pathname was to be created but the device containing pathname has  no 
>  room  for  the  new file.
> EOVERFLOW:
>  pathname refers to a regular file that is too large to be opened.
> (snip)

> Signed-off-by: Satoru Takeuchi 
> ---
> This patch can be applied to 4.5-rc7
> ---
>  fs/btrfs/inode-map.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/inode-map.c b/fs/btrfs/inode-map.c
> index e50316c..f5e3228 100644
> --- a/fs/btrfs/inode-map.c
> +++ b/fs/btrfs/inode-map.c
> @@ -556,7 +556,15 @@ int btrfs_find_free_objectid(struct btrfs_root *root, 
> u64 *objectid)
> mutex_lock(&root->objectid_mutex);
>
> if (unlikely(root->highest_objectid >= BTRFS_LAST_FREE_OBJECTID)) {
> -   ret = -ENOSPC;
> +   static bool __warned = false;
> +
> +   if (unlikely(!__warned)) {
> +   btrfs_warn(root->fs_info,
> +  "The objectid of root %llu reaches its 
> highest value.\n",
> +  root->root_key.objectid);
> +   __warned = true;
> +   }
> +   ret = -EOVERFLOW;
> goto out;
> }
>
> --
> 2.5.0
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Btrfs: Show a warning message if one of objectid reaches its highest value

2016-03-08 Thread Goffredo Baroncelli 
On 2016-03-07 04:05, Satoru Takeuchi wrote:
> - It's better to show a warning message for the exceptional case
>that one of objectid (in most case, inode number) reaches its
>highest value. Show this message only once to avoid filling
>dmesg with it.
> - EOVERFLOW is more proper return value for this case.
>ENOSPC is for "No space left on device" case and objectid isn't
>related to any device.
> 
> Signed-off-by: Satoru Takeuchi 
> ---
> This patch can be applied to 4.5-rc7
> ---
>   fs/btrfs/inode-map.c | 10 +-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode-map.c b/fs/btrfs/inode-map.c
> index e50316c..f5e3228 100644
> --- a/fs/btrfs/inode-map.c
> +++ b/fs/btrfs/inode-map.c
> @@ -556,7 +556,15 @@ int btrfs_find_free_objectid(struct btrfs_root *root, 
> u64 *objectid)
>   mutex_lock(&root->objectid_mutex);
> 
>   if (unlikely(root->highest_objectid >= BTRFS_LAST_FREE_OBJECTID)) {
> - ret = -ENOSPC;
> + static bool __warned = false;


Please, don't use a static GLOBAL variable. I suggest to move to a "per 
filesystem" variables for two main reasons:

1) if in the (very unlikely) case where two different filesystems reach 
BTRFS_LAST_FREE_OBJECTID, the first error will hide the second one.

2) if you umount and remount the filesystem the error is not shown anymore. A 
module unload/load or a reboot is required. 
If something strange happens, one of the first thing that the user does, is to 
umount/remount the filesystem. But the error is not show anymore. This could 
complicate the diagnosis of the problem. 




> +
> + if (unlikely(!__warned)) {
> + btrfs_warn(root->fs_info,
> +"The objectid of root %llu reaches its 
> highest value.\n",
> +root->root_key.objectid);
> + __warned = true;
> + }
> + ret = -EOVERFLOW;
>   goto out;
>   }
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli 
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Btrfs: Show a warning message if one of objectid reaches its highest value

2016-03-06 Thread Satoru Takeuchi 
On 2016/03/07 12:05, Satoru Takeuchi wrote:
> - It's better to show a warning message for the exceptional case
>that one of objectid (in most case, inode number) reaches its
>highest value. Show this message only once to avoid filling
>dmesg with it.
> - EOVERFLOW is more proper return value for this case.
>ENOSPC is for "No space left on device" case and objectid isn't
>related to any device.
> 
> Signed-off-by: Satoru Takeuchi 
> ---
> This patch can be applied to 4.5-rc7

Sorry, I forgot to add changelog.

===
V2:
 - Reflect Filipe's comment
   - Use btrfs_warn() instead of WARN_ONCE()
   - Print the id of the tree
===

Thanks,
Satoru

> ---
>   fs/btrfs/inode-map.c | 10 +-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode-map.c b/fs/btrfs/inode-map.c
> index e50316c..f5e3228 100644
> --- a/fs/btrfs/inode-map.c
> +++ b/fs/btrfs/inode-map.c
> @@ -556,7 +556,15 @@ int btrfs_find_free_objectid(struct btrfs_root *root, 
> u64 *objectid)
>   mutex_lock(&root->objectid_mutex);
> 
>   if (unlikely(root->highest_objectid >= BTRFS_LAST_FREE_OBJECTID)) {
> - ret = -ENOSPC;
> + static bool __warned = false;
> +
> + if (unlikely(!__warned)) {
> + btrfs_warn(root->fs_info,
> +"The objectid of root %llu reaches its 
> highest value.\n",
> +root->root_key.objectid);
> + __warned = true;
> + }
> + ret = -EOVERFLOW;
>   goto out;
>   }
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] Btrfs: Show a warning message if one of objectid reaches its highest value

2016-03-06 Thread Satoru Takeuchi 
- It's better to show a warning message for the exceptional case
  that one of objectid (in most case, inode number) reaches its
  highest value. Show this message only once to avoid filling
  dmesg with it.
- EOVERFLOW is more proper return value for this case.
  ENOSPC is for "No space left on device" case and objectid isn't
  related to any device.

Signed-off-by: Satoru Takeuchi 
---
This patch can be applied to 4.5-rc7
---
 fs/btrfs/inode-map.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode-map.c b/fs/btrfs/inode-map.c
index e50316c..f5e3228 100644
--- a/fs/btrfs/inode-map.c
+++ b/fs/btrfs/inode-map.c
@@ -556,7 +556,15 @@ int btrfs_find_free_objectid(struct btrfs_root *root, u64 
*objectid)
mutex_lock(&root->objectid_mutex);

if (unlikely(root->highest_objectid >= BTRFS_LAST_FREE_OBJECTID)) {
-   ret = -ENOSPC;
+   static bool __warned = false;
+
+   if (unlikely(!__warned)) {
+   btrfs_warn(root->fs_info,
+  "The objectid of root %llu reaches its 
highest value.\n",
+  root->root_key.objectid);
+   __warned = true;
+   }
+   ret = -EOVERFLOW;
goto out;
}

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