[PATCH] Btrfs-progs: check out if the swap device

[PATCH] Btrfs-progs: check out if the swap device

[Tsutomu Itoh]

Currently, the following commands succeed.

 # cat /proc/swaps
 Filename                                Type            Size    Used    Priority
 /dev/sda3                               partition       8388604 0       -1
 /dev/sdc8                               partition       9765884 0       -2
 # mkfs.btrfs /dev/sdc8
 
 WARNING! - Btrfs v0.20-rc1-165-g82ac345 IS EXPERIMENTAL
 WARNING! - see http://btrfs.wiki.kernel.org before using
 
 fs created label (null) on /dev/sdc8
         nodesize 4096 leafsize 4096 sectorsize 4096 size 9.31GB
 Btrfs v0.20-rc1-165-g82ac345
 # btrfs fi sh /dev/sdc8
 Label: none  uuid: fc0bdbd0-7eed-460f-b4e9-131273b66df2
         Total devices 1 FS bytes used 28.00KB
         devid    1 size 9.31GB used 989.62MB path /dev/sdc8
 
 Btrfs v0.20-rc1-165-g82ac345
 #

But we should check out the swap device. So fixed it.

Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
---
(this patch is based on Chris's raid56-experimental branch)
---
 mkfs.c  | 18 ++++++++++++++++++
 utils.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 utils.h |  1 +
 3 files changed, 68 insertions(+)

diff --git a/mkfs.c b/mkfs.c
index 2d3c2af..fdc3373 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -1366,6 +1366,15 @@ int main(int ac, char **av)
 
 	if (source_dir == 0) {
 		file = av[optind++];
+		ret = is_swap_device(file);
+		if (ret < 0) {
+			fprintf(stderr, "error checking %s status\n", file);
+			exit(1);
+		}
+		if (ret == 1) {
+			fprintf(stderr, "%s is a swap device\n", file);
+			exit(1);
+		}
 		ret = check_mounted(file);
 		if (ret < 0) {
 			fprintf(stderr, "error checking %s mount status\n", file);
@@ -1461,6 +1470,15 @@ int main(int ac, char **av)
 		int old_mixed = mixed;
 
 		file = av[optind++];
+		ret = is_swap_device(file);
+		if (ret < 0) {
+			fprintf(stderr, "error checking %s status\n", file);
+			exit(1);
+		}
+		if (ret == 1) {
+			fprintf(stderr, "%s is a swap device\n", file);
+			exit(1);
+		}
 		ret = check_mounted(file);
 		if (ret < 0) {
 			fprintf(stderr, "error checking %s mount status\n",
diff --git a/utils.c b/utils.c
index f9ee812..0c551a0 100644
--- a/utils.c
+++ b/utils.c
@@ -1386,3 +1386,52 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 
 	return 0;
 }
+
+/*
+ * Checks if the swap device or not.
+ * Returns 1 if the swap device, < 0 on error or 0 if not the swap device.
+ */
+int is_swap_device(const char *file)
+{
+	FILE	*f;
+	struct stat	st_buf;
+	char	buf[1024];
+	char	*cp;
+	dev_t	rdev;
+	int	ret = 0;
+
+	if (stat(file, &st_buf) < 0)
+		return -errno;
+	if (!S_ISBLK(st_buf.st_mode))
+		return 0;
+
+	rdev = st_buf.st_rdev;
+
+	if ((f = fopen("/proc/swaps", "r")) == NULL)
+		return -errno;
+
+	/* skip the first line */
+	if (fgets(buf, sizeof(buf), f) == NULL)
+		goto out;
+
+	while (fgets(buf, sizeof(buf), f) != NULL) {
+		if ((cp = strchr(buf, ' ')) != NULL)
+			*cp = 0;
+		if ((cp = strchr(buf, '\t')) != NULL)
+			*cp = 0;
+		if (strcmp(file, buf) == 0) {
+			ret = 1;
+			break;
+		}
+		if ((stat(buf, &st_buf) == 0) && S_ISBLK(st_buf.st_mode) &&
+		    rdev == st_buf.st_rdev) {
+			ret = 1;
+			break;
+		}
+	}
+
+out:
+	fclose(f);
+
+	return ret;
+}
diff --git a/utils.h b/utils.h
index bbcaf6a..60a0fea 100644
--- a/utils.h
+++ b/utils.h
@@ -55,6 +55,7 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 		struct btrfs_ioctl_dev_info_args **di_ret);
 
 char *__strncpy__null(char *dest, const char *src, size_t n);
+int is_swap_device(const char *file);
 /* Helper to always get proper size of the destination string */
 #define strncpy_null(dest, src) __strncpy__null(dest, src, sizeof(dest))
 

Re: [PATCH] Btrfs-progs: check out if the swap device

[Eric Sandeen]

On 2/11/13 7:25 PM, Tsutomu Itoh wrote:
> Currently, the following commands succeed.
> 
>  # cat /proc/swaps
>  Filename                                Type            Size    Used    Priority
>  /dev/sda3                               partition       8388604 0       -1
>  /dev/sdc8                               partition       9765884 0       -2
>  # mkfs.btrfs /dev/sdc8
>  
>  WARNING! - Btrfs v0.20-rc1-165-g82ac345 IS EXPERIMENTAL
>  WARNING! - see http://btrfs.wiki.kernel.org before using
>  
>  fs created label (null) on /dev/sdc8
>          nodesize 4096 leafsize 4096 sectorsize 4096 size 9.31GB
>  Btrfs v0.20-rc1-165-g82ac345
>  # btrfs fi sh /dev/sdc8
>  Label: none  uuid: fc0bdbd0-7eed-460f-b4e9-131273b66df2
>          Total devices 1 FS bytes used 28.00KB
>          devid    1 size 9.31GB used 989.62MB path /dev/sdc8
>  
>  Btrfs v0.20-rc1-165-g82ac345
>  #
> 
> But we should check out the swap device. So fixed it.

I guess it's nice to parse /proc/swaps to be able to offer the
helpful error message in this case.  (though I wonder how long
/proc/swaps will be available, and in this format?  Does it count
as ABI?)

Your implementation looks just like the one in e2fsprogs, so it should
work fine.

But I also wonder if overall it would be safest to open the device O_EXCL,
which would fail with EBUSY if it were in use for swap, or mounted,
or opened O_EXCL by another process for any other reason:

[root@host tmp]# cat /proc/swaps
Filename				Type		Size	Used	Priority
/dev/sda3                               partition	2048280	822616	-1

[root@host tmp]# strace -e open ./test
open("/etc/ld.so.cache", O_RDONLY)      = 3
open("/lib64/libc.so.6", O_RDONLY)      = 3
open("/dev/sda3", O_RDWR|O_EXCL)        = -1 EBUSY (Device or resource busy)
open: Device or resource busy

-Eric

> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
> ---
> (this patch is based on Chris's raid56-experimental branch)
> ---
>  mkfs.c  | 18 ++++++++++++++++++
>  utils.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  utils.h |  1 +
>  3 files changed, 68 insertions(+)
> 
> diff --git a/mkfs.c b/mkfs.c
> index 2d3c2af..fdc3373 100644
> --- a/mkfs.c
> +++ b/mkfs.c
> @@ -1366,6 +1366,15 @@ int main(int ac, char **av)
>  
>  	if (source_dir == 0) {
>  		file = av[optind++];
> +		ret = is_swap_device(file);
> +		if (ret < 0) {
> +			fprintf(stderr, "error checking %s status\n", file);
> +			exit(1);
> +		}
> +		if (ret == 1) {
> +			fprintf(stderr, "%s is a swap device\n", file);
> +			exit(1);
> +		}
>  		ret = check_mounted(file);
>  		if (ret < 0) {
>  			fprintf(stderr, "error checking %s mount status\n", file);
> @@ -1461,6 +1470,15 @@ int main(int ac, char **av)
>  		int old_mixed = mixed;
>  
>  		file = av[optind++];
> +		ret = is_swap_device(file);
> +		if (ret < 0) {
> +			fprintf(stderr, "error checking %s status\n", file);
> +			exit(1);
> +		}
> +		if (ret == 1) {
> +			fprintf(stderr, "%s is a swap device\n", file);
> +			exit(1);
> +		}
>  		ret = check_mounted(file);
>  		if (ret < 0) {
>  			fprintf(stderr, "error checking %s mount status\n",
> diff --git a/utils.c b/utils.c
> index f9ee812..0c551a0 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1386,3 +1386,52 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>  
>  	return 0;
>  }
> +
> +/*
> + * Checks if the swap device or not.
> + * Returns 1 if the swap device, < 0 on error or 0 if not the swap device.
> + */
> +int is_swap_device(const char *file)
> +{
> +	FILE	*f;
> +	struct stat	st_buf;
> +	char	buf[1024];
> +	char	*cp;
> +	dev_t	rdev;
> +	int	ret = 0;
> +
> +	if (stat(file, &st_buf) < 0)
> +		return -errno;
> +	if (!S_ISBLK(st_buf.st_mode))
> +		return 0;
> +
> +	rdev = st_buf.st_rdev;
> +
> +	if ((f = fopen("/proc/swaps", "r")) == NULL)
> +		return -errno;
> +
> +	/* skip the first line */
> +	if (fgets(buf, sizeof(buf), f) == NULL)
> +		goto out;
> +
> +	while (fgets(buf, sizeof(buf), f) != NULL) {
> +		if ((cp = strchr(buf, ' ')) != NULL)
> +			*cp = 0;
> +		if ((cp = strchr(buf, '\t')) != NULL)
> +			*cp = 0;
> +		if (strcmp(file, buf) == 0) {
> +			ret = 1;
> +			break;
> +		}
> +		if ((stat(buf, &st_buf) == 0) && S_ISBLK(st_buf.st_mode) &&
> +		    rdev == st_buf.st_rdev) {
> +			ret = 1;
> +			break;
> +		}
> +	}
> +
> +out:
> +	fclose(f);
> +
> +	return ret;
> +}
> diff --git a/utils.h b/utils.h
> index bbcaf6a..60a0fea 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -55,6 +55,7 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>  		struct btrfs_ioctl_dev_info_args **di_ret);
>  
>  char *__strncpy__null(char *dest, const char *src, size_t n);
> +int is_swap_device(const char *file);
>  /* Helper to always get proper size of the destination string */
>  #define strncpy_null(dest, src) __strncpy__null(dest, src, sizeof(dest))
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] Btrfs-progs: check out if the swap device

[Tsutomu Itoh]

Hi, Eric,

Thanks for your comment.

On 2013/02/12 13:22, Eric Sandeen wrote:
> On 2/11/13 7:25 PM, Tsutomu Itoh wrote:
>> Currently, the following commands succeed.
>>
>>   # cat /proc/swaps
>>   Filename                                Type            Size    Used    Priority
>>   /dev/sda3                               partition       8388604 0       -1
>>   /dev/sdc8                               partition       9765884 0       -2
>>   # mkfs.btrfs /dev/sdc8
>>
>>   WARNING! - Btrfs v0.20-rc1-165-g82ac345 IS EXPERIMENTAL
>>   WARNING! - see http://btrfs.wiki.kernel.org before using
>>
>>   fs created label (null) on /dev/sdc8
>>           nodesize 4096 leafsize 4096 sectorsize 4096 size 9.31GB
>>   Btrfs v0.20-rc1-165-g82ac345
>>   # btrfs fi sh /dev/sdc8
>>   Label: none  uuid: fc0bdbd0-7eed-460f-b4e9-131273b66df2
>>           Total devices 1 FS bytes used 28.00KB
>>           devid    1 size 9.31GB used 989.62MB path /dev/sdc8
>>
>>   Btrfs v0.20-rc1-165-g82ac345
>>   #
>>
>> But we should check out the swap device. So fixed it.
>
> I guess it's nice to parse /proc/swaps to be able to offer the

> helpful error message in this case.  (though I wonder how long
> /proc/swaps will be available, and in this format?  Does it count
> as ABI?)

Umm, I don't know how long /proc/swaps will be available, too...

>
> Your implementation looks just like the one in e2fsprogs, so it should
> work fine.

Yes.

>
> But I also wonder if overall it would be safest to open the device O_EXCL,
> which would fail with EBUSY if it were in use for swap, or mounted,
> or opened O_EXCL by another process for any other reason:

But details of the error cannot be notified when O_EXCL is used. and,
after is_swap_device(), check_mounted() check state of the mount or not.

So, I chose this one. (read /proc/swaps)

Thanks,
Tsutomu

>
> [root@host tmp]# cat /proc/swaps
> Filename				Type		Size	Used	Priority
> /dev/sda3                               partition	2048280	822616	-1
>
> [root@host tmp]# strace -e open ./test
> open("/etc/ld.so.cache", O_RDONLY)      = 3
> open("/lib64/libc.so.6", O_RDONLY)      = 3
> open("/dev/sda3", O_RDWR|O_EXCL)        = -1 EBUSY (Device or resource busy)
> open: Device or resource busy
>
> -Eric
>
>> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
>> ---
>> (this patch is based on Chris's raid56-experimental branch)
>> ---
>>   mkfs.c  | 18 ++++++++++++++++++
>>   utils.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   utils.h |  1 +
>>   3 files changed, 68 insertions(+)
>>
>> diff --git a/mkfs.c b/mkfs.c
>> index 2d3c2af..fdc3373 100644
>> --- a/mkfs.c
>> +++ b/mkfs.c
>> @@ -1366,6 +1366,15 @@ int main(int ac, char **av)
>>
>>   	if (source_dir == 0) {
>>   		file = av[optind++];
>> +		ret = is_swap_device(file);
>> +		if (ret < 0) {
>> +			fprintf(stderr, "error checking %s status\n", file);
>> +			exit(1);
>> +		}
>> +		if (ret == 1) {
>> +			fprintf(stderr, "%s is a swap device\n", file);
>> +			exit(1);
>> +		}
>>   		ret = check_mounted(file);
>>   		if (ret < 0) {
>>   			fprintf(stderr, "error checking %s mount status\n", file);
>> @@ -1461,6 +1470,15 @@ int main(int ac, char **av)
>>   		int old_mixed = mixed;
>>
>>   		file = av[optind++];
>> +		ret = is_swap_device(file);
>> +		if (ret < 0) {
>> +			fprintf(stderr, "error checking %s status\n", file);
>> +			exit(1);
>> +		}
>> +		if (ret == 1) {
>> +			fprintf(stderr, "%s is a swap device\n", file);
>> +			exit(1);
>> +		}
>>   		ret = check_mounted(file);
>>   		if (ret < 0) {
>>   			fprintf(stderr, "error checking %s mount status\n",
>> diff --git a/utils.c b/utils.c
>> index f9ee812..0c551a0 100644
>> --- a/utils.c
>> +++ b/utils.c
>> @@ -1386,3 +1386,52 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>>
>>   	return 0;
>>   }
>> +
>> +/*
>> + * Checks if the swap device or not.
>> + * Returns 1 if the swap device, < 0 on error or 0 if not the swap device.
>> + */
>> +int is_swap_device(const char *file)
>> +{
>> +	FILE	*f;
>> +	struct stat	st_buf;
>> +	char	buf[1024];
>> +	char	*cp;
>> +	dev_t	rdev;
>> +	int	ret = 0;
>> +
>> +	if (stat(file, &st_buf) < 0)
>> +		return -errno;
>> +	if (!S_ISBLK(st_buf.st_mode))
>> +		return 0;
>> +
>> +	rdev = st_buf.st_rdev;
>> +
>> +	if ((f = fopen("/proc/swaps", "r")) == NULL)
>> +		return -errno;
>> +
>> +	/* skip the first line */
>> +	if (fgets(buf, sizeof(buf), f) == NULL)
>> +		goto out;
>> +
>> +	while (fgets(buf, sizeof(buf), f) != NULL) {
>> +		if ((cp = strchr(buf, ' ')) != NULL)
>> +			*cp = 0;
>> +		if ((cp = strchr(buf, '\t')) != NULL)
>> +			*cp = 0;
>> +		if (strcmp(file, buf) == 0) {
>> +			ret = 1;
>> +			break;
>> +		}
>> +		if ((stat(buf, &st_buf) == 0) && S_ISBLK(st_buf.st_mode) &&
>> +		    rdev == st_buf.st_rdev) {
>> +			ret = 1;
>> +			break;
>> +		}
>> +	}
>> +
>> +out:
>> +	fclose(f);
>> +
>> +	return ret;
>> +}
>> diff --git a/utils.h b/utils.h
>> index bbcaf6a..60a0fea 100644
>> --- a/utils.h
>> +++ b/utils.h
>> @@ -55,6 +55,7 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>>   		struct btrfs_ioctl_dev_info_args **di_ret);
>>
>>   char *__strncpy__null(char *dest, const char *src, size_t n);
>> +int is_swap_device(const char *file);
>>   /* Helper to always get proper size of the destination string */
>>   #define strncpy_null(dest, src) __strncpy__null(dest, src, sizeof(dest))
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: [PATCH] Btrfs-progs: check out if the swap device

[Eric Sandeen]

On 2/11/13 11:50 PM, Tsutomu Itoh wrote:
> Hi, Eric,
> 
> Thanks for your comment.
> 
> On 2013/02/12 13:22, Eric Sandeen wrote:
>> On 2/11/13 7:25 PM, Tsutomu Itoh wrote:
>>> Currently, the following commands succeed.
>>>
>>>   # cat /proc/swaps
>>>   Filename                                Type            Size    Used    Priority
>>>   /dev/sda3                               partition       8388604 0       -1
>>>   /dev/sdc8                               partition       9765884 0       -2
>>>   # mkfs.btrfs /dev/sdc8
>>>
>>>   WARNING! - Btrfs v0.20-rc1-165-g82ac345 IS EXPERIMENTAL
>>>   WARNING! - see http://btrfs.wiki.kernel.org before using
>>>
>>>   fs created label (null) on /dev/sdc8
>>>           nodesize 4096 leafsize 4096 sectorsize 4096 size 9.31GB
>>>   Btrfs v0.20-rc1-165-g82ac345
>>>   # btrfs fi sh /dev/sdc8
>>>   Label: none  uuid: fc0bdbd0-7eed-460f-b4e9-131273b66df2
>>>           Total devices 1 FS bytes used 28.00KB
>>>           devid    1 size 9.31GB used 989.62MB path /dev/sdc8
>>>
>>>   Btrfs v0.20-rc1-165-g82ac345
>>>   #
>>>
>>> But we should check out the swap device. So fixed it.
>>
>> I guess it's nice to parse /proc/swaps to be able to offer the
> 
>> helpful error message in this case.  (though I wonder how long
>> /proc/swaps will be available, and in this format?  Does it count
>> as ABI?)
> 
> Umm, I don't know how long /proc/swaps will be available, too...

I guess it is good enough for e2fsprogs :)

>>
>> Your implementation looks just like the one in e2fsprogs, so it should
>> work fine.
> 
> Yes.
> 
>>
>> But I also wonder if overall it would be safest to open the device O_EXCL,
>> which would fail with EBUSY if it were in use for swap, or mounted,
>> or opened O_EXCL by another process for any other reason:
> 
> But details of the error cannot be notified when O_EXCL is used. and,
> after is_swap_device(), check_mounted() check state of the mount or not.
> 
> So, I chose this one. (read /proc/swaps)

Sure, I think your change is good.  I just think perhaps mkfs should also try
to open O_EXCL after all those other tests, as a last safety check.

I can take a look at the code & send that patch if it seems to make sense.

Thanks,
-Eric
 
> Thanks,
> Tsutomu
> 
>>
>> [root@host tmp]# cat /proc/swaps
>> Filename                Type        Size    Used    Priority
>> /dev/sda3                               partition    2048280    822616    -1
>>
>> [root@host tmp]# strace -e open ./test
>> open("/etc/ld.so.cache", O_RDONLY)      = 3
>> open("/lib64/libc.so.6", O_RDONLY)      = 3
>> open("/dev/sda3", O_RDWR|O_EXCL)        = -1 EBUSY (Device or resource busy)
>> open: Device or resource busy
>>
>> -Eric
>>
>>> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
>>> ---
>>> (this patch is based on Chris's raid56-experimental branch)
>>> ---
>>>   mkfs.c  | 18 ++++++++++++++++++
>>>   utils.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>   utils.h |  1 +
>>>   3 files changed, 68 insertions(+)
>>>
>>> diff --git a/mkfs.c b/mkfs.c
>>> index 2d3c2af..fdc3373 100644
>>> --- a/mkfs.c
>>> +++ b/mkfs.c
>>> @@ -1366,6 +1366,15 @@ int main(int ac, char **av)
>>>
>>>       if (source_dir == 0) {
>>>           file = av[optind++];
>>> +        ret = is_swap_device(file);
>>> +        if (ret < 0) {
>>> +            fprintf(stderr, "error checking %s status\n", file);
>>> +            exit(1);
>>> +        }
>>> +        if (ret == 1) {
>>> +            fprintf(stderr, "%s is a swap device\n", file);
>>> +            exit(1);
>>> +        }
>>>           ret = check_mounted(file);
>>>           if (ret < 0) {
>>>               fprintf(stderr, "error checking %s mount status\n", file);
>>> @@ -1461,6 +1470,15 @@ int main(int ac, char **av)
>>>           int old_mixed = mixed;
>>>
>>>           file = av[optind++];
>>> +        ret = is_swap_device(file);
>>> +        if (ret < 0) {
>>> +            fprintf(stderr, "error checking %s status\n", file);
>>> +            exit(1);
>>> +        }
>>> +        if (ret == 1) {
>>> +            fprintf(stderr, "%s is a swap device\n", file);
>>> +            exit(1);
>>> +        }
>>>           ret = check_mounted(file);
>>>           if (ret < 0) {
>>>               fprintf(stderr, "error checking %s mount status\n",
>>> diff --git a/utils.c b/utils.c
>>> index f9ee812..0c551a0 100644
>>> --- a/utils.c
>>> +++ b/utils.c
>>> @@ -1386,3 +1386,52 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>>>
>>>       return 0;
>>>   }
>>> +
>>> +/*
>>> + * Checks if the swap device or not.
>>> + * Returns 1 if the swap device, < 0 on error or 0 if not the swap device.
>>> + */
>>> +int is_swap_device(const char *file)
>>> +{
>>> +    FILE    *f;
>>> +    struct stat    st_buf;
>>> +    char    buf[1024];
>>> +    char    *cp;
>>> +    dev_t    rdev;
>>> +    int    ret = 0;
>>> +
>>> +    if (stat(file, &st_buf) < 0)
>>> +        return -errno;
>>> +    if (!S_ISBLK(st_buf.st_mode))
>>> +        return 0;
>>> +
>>> +    rdev = st_buf.st_rdev;
>>> +
>>> +    if ((f = fopen("/proc/swaps", "r")) == NULL)
>>> +        return -errno;
>>> +
>>> +    /* skip the first line */
>>> +    if (fgets(buf, sizeof(buf), f) == NULL)
>>> +        goto out;
>>> +
>>> +    while (fgets(buf, sizeof(buf), f) != NULL) {
>>> +        if ((cp = strchr(buf, ' ')) != NULL)
>>> +            *cp = 0;
>>> +        if ((cp = strchr(buf, '\t')) != NULL)
>>> +            *cp = 0;
>>> +        if (strcmp(file, buf) == 0) {
>>> +            ret = 1;
>>> +            break;
>>> +        }
>>> +        if ((stat(buf, &st_buf) == 0) && S_ISBLK(st_buf.st_mode) &&
>>> +            rdev == st_buf.st_rdev) {
>>> +            ret = 1;
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +out:
>>> +    fclose(f);
>>> +
>>> +    return ret;
>>> +}
>>> diff --git a/utils.h b/utils.h
>>> index bbcaf6a..60a0fea 100644
>>> --- a/utils.h
>>> +++ b/utils.h
>>> @@ -55,6 +55,7 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>>>           struct btrfs_ioctl_dev_info_args **di_ret);
>>>
>>>   char *__strncpy__null(char *dest, const char *src, size_t n);
>>> +int is_swap_device(const char *file);
>>>   /* Helper to always get proper size of the destination string */
>>>   #define strncpy_null(dest, src) __strncpy__null(dest, src, sizeof(dest))
>>>
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> 
> 

Re: [PATCH] Btrfs-progs: check out if the swap device

[Zach Brown]

> > 
> > So, I chose this one. (read /proc/swaps)
> 
> Sure, I think your change is good.  I just think perhaps mkfs should also try
> to open O_EXCL after all those other tests, as a last safety check.

I think mkfs should first try an O_EXCL open.  If that works it doesn't
need to do any of this work to try and predict if the open will fail.

After it fails it can poke around a bit to try and give nice context for
why it failed.  But it might not be able to because /proc/swaps is
fundamentally unreliable.

> >>>           file = av[optind++];
> >>> +        ret = is_swap_device(file);

The input string is a CWD-realtive path.  You'd at least want to use
realpath() to get it to a canonical name.  So it's not full of junk like
"./" and "../../.." which won't be present in /proc/swaps.

> >>> +    char    buf[1024];

Use PATH_MAX so it doesn't fail on absurd but allowed file names.
(Where on earth does 1024 come from?)

> >>> +    if ((f = fopen("/proc/swaps", "r")) == NULL)
> >>> +        return -errno;

As was pointed out, there's no reason this failure should stop mkfs.
/proc/swaps might not be available or allowed and I should still be able
to do an unpriviledged "./mkfs ./myfile; ./btrfs-debug-tree ./myfile".

> >>> +        if (strcmp(file, buf) == 0) {
> >>> +            ret = 1;
> >>> +            break;
> >>> +        }

The command line path that lead to the inode might not be the path for
the inode that is in /proc/swaps.  Bind mounts, chroot, name spaces,
hard links, etc, all make it possible -- though unlikely -- that mkfs
simply won't be able to tell if the path names are related.

Also, /proc/swaps escapes whitespace in file names.  So you could be
comparing "swap file" with "swap\040file".

> >>> +        if ((stat(buf, &st_buf) == 0) && S_ISBLK(st_buf.st_mode) &&
> >>> +            rdev == st_buf.st_rdev) {
> >>> +            ret = 1;
> >>> +            break;
> >>> +        }

One possible alternative is to then try and open every swap file path to
see if it points to the same inode as the path mkfs was given.  But you
might not have access to the paths and we're back to the unpriviledged
failure case.

- z

Re: [PATCH] Btrfs-progs: check out if the swap device

[Tsutomu Itoh]

Hi, All,

Thanks for advice.

On 2013/02/13 5:57, Zach Brown wrote:
>>>
>>> So, I chose this one. (read /proc/swaps)
>>
>> Sure, I think your change is good.  I just think perhaps mkfs should also try
>> to open O_EXCL after all those other tests, as a last safety check.
>
> I think mkfs should first try an O_EXCL open.  If that works it doesn't
> need to do any of this work to try and predict if the open will fail.
>
> After it fails it can poke around a bit to try and give nice context for

> why it failed.  But it might not be able to because /proc/swaps is
> fundamentally unreliable.

Then, how should we do?    I have no idea...

>
>>>>>            file = av[optind++];
>>>>> +        ret = is_swap_device(file);
>
> The input string is a CWD-realtive path.  You'd at least want to use
> realpath() to get it to a canonical name.  So it's not full of junk like
> "./" and "../../.." which won't be present in /proc/swaps.
>
>>>>> +    char    buf[1024];
>
> Use PATH_MAX so it doesn't fail on absurd but allowed file names.
> (Where on earth does 1024 come from?)
>
>>>>> +    if ((f = fopen("/proc/swaps", "r")) == NULL)
>>>>> +        return -errno;
>
> As was pointed out, there's no reason this failure should stop mkfs.
> /proc/swaps might not be available or allowed and I should still be able
> to do an unpriviledged "./mkfs ./myfile; ./btrfs-debug-tree ./myfile".
>
>>>>> +        if (strcmp(file, buf) == 0) {
>>>>> +            ret = 1;
>>>>> +            break;
>>>>> +        }
>
> The command line path that lead to the inode might not be the path for
> the inode that is in /proc/swaps.  Bind mounts, chroot, name spaces,
> hard links, etc, all make it possible -- though unlikely -- that mkfs
> simply won't be able to tell if the path names are related.

OK. I agree.

 >
> Also, /proc/swaps escapes whitespace in file names.  So you could be
> comparing "swap file" with "swap\040file".

I had forgotten this. Thank you for pointing it out.

>
>>>>> +        if ((stat(buf, &st_buf) == 0) && S_ISBLK(st_buf.st_mode) &&
>>>>> +            rdev == st_buf.st_rdev) {
>>>>> +            ret = 1;
>>>>> +            break;
>>>>> +        }
>
> One possible alternative is to then try and open every swap file path to
> see if it points to the same inode as the path mkfs was given.  But you
> might not have access to the paths and we're back to the unpriviledged
> failure case.

I want to think about a new patch later. and, I will post it again if it
seems to make a sense.

Thanks,
Tsutomu

Re: [PATCH] Btrfs-progs: check out if the swap device

[Zach Brown]

> >why it failed.  But it might not be able to because /proc/swaps is
> >fundamentally unreliable.
> 
> Then, how should we do?    I have no idea...

Hmm.  I think I'd do something like:

- First always open with O_EXCL.  If it succeeds then there's no reason
  to check /proc/swaps at all.  (Maybe it doesn't need to try
  check_mounted() there either?  Not sure if it's protecting against 
  accidentally mounting mounted shared storage or not.)

- Try stat()ing the /proc/swaps paths and the command line path.  If they
  point to the same inode then print a helpful message that the open
  might have failed because the file is an active swap file.

- Use realpath() to resolve the relative path into an absolute path.
  Copy it and escape control chars ("\n\t\\") with their \0xxx octal
  equivalents.  If the mangled absolute path matches the path in
  /proc/swaps (without opening), print the helpful message.

- At no point is failure of any of the /proc/swaps parsing fatal.  It'd
  carry on ignoring errors until it doesnt have work to do.  It'd only
  ever print the nice message when it finds a match.
  
That seems reasonable to me.  It costs nothing in the vast majority of
invocations when nothing goes wrong, it doesn't *cause* problems, and
it'd print helpful messages on boring normal systems when someone really
does accidentally try and mkfs a swapfile.

In very rare cases /proc/swaps won't be of any help.  The user would
only see the open failure.  That's fine, it's just not worth worrying
about.

- z

Re: [PATCH] Btrfs-progs: check out if the swap device

[Tsutomu Itoh]

On 2013/02/14 6:58, Zach Brown wrote:
>>> why it failed.  But it might not be able to because /proc/swaps is
>>> fundamentally unreliable.
>>
>> Then, how should we do?    I have no idea...
>
> Hmm.  I think I'd do something like:
>
> - First always open with O_EXCL.  If it succeeds then there's no reason
>    to check /proc/swaps at all.  (Maybe it doesn't need to try
>    check_mounted() there either?  Not sure if it's protecting against
>    accidentally mounting mounted shared storage or not.)

check_mounted() is necessary for multiple devices, I think.

>
> - Try stat()ing the /proc/swaps paths and the command line path.  If they
>    point to the same inode then print a helpful message that the open
>    might have failed because the file is an active swap file.
>
> - Use realpath() to resolve the relative path into an absolute path.
>    Copy it and escape control chars ("\n\t\\") with their \0xxx octal
>    equivalents.  If the mangled absolute path matches the path in
>    /proc/swaps (without opening), print the helpful message.

I think realpath() is unnecessary if it checks it by using only
stat() information. (if do not compare path)

Am I misunderstanding anything?

Thanks,
Tsutomu

>
> - At no point is failure of any of the /proc/swaps parsing fatal.  It'd
>    carry on ignoring errors until it doesnt have work to do.  It'd only
>    ever print the nice message when it finds a match.
>
> That seems reasonable to me.  It costs nothing in the vast majority of
> invocations when nothing goes wrong, it doesn't *cause* problems, and
> it'd print helpful messages on boring normal systems when someone really
> does accidentally try and mkfs a swapfile.
>
> In very rare cases /proc/swaps won't be of any help.  The user would
> only see the open failure.  That's fine, it's just not worth worrying
> about.
>
> - z

Re: [PATCH] Btrfs-progs: check out if the swap device

[Zach Brown]

> I think realpath() is unnecessary if it checks it by using only
> stat() information. (if do not compare path)
> 
> Am I misunderstanding anything?

Sure, that'd be fine, but then you'd want to try unescaping the paths
before stati()ng them.

- z

Re: [PATCH] Btrfs-progs: check out if the swap device

[David Sterba]

On Tue, Feb 12, 2013 at 10:25:23AM +0900, Tsutomu Itoh wrote:
> Currently, the following commands succeed.
> 
>  # cat /proc/swaps
>  Filename                                Type            Size    Used    Priority
>  /dev/sda3                               partition       8388604 0       -1
>  /dev/sdc8                               partition       9765884 0       -2
>  # mkfs.btrfs /dev/sdc8

if a swap device is backed by a file, mkfs succeeds:

Filename                                Type            Size    Used
Priority
/dev/sda15                              partition       4232016 0 -2
/mnt/swap                               file            10236   0 -3

$ mkfs.btrfs /mnt/swap

WARNING! - Btrfs 0.20 IS EXPERIMENTAL
WARNING! - see http://btrfs.wiki.kernel.org before using

SMALL VOLUME: forcing mixed metadata/data groups
Created a data/metadata chunk of size 1048576
fs created label (null) on /mnt/swap
        nodesize 4096 leafsize 4096 sectorsize 4096 size 10.00MB
Btrfs 0.20
---

Please add check for this as well.

thanks,
david

Re: [PATCH] Btrfs-progs: check out if the swap device

[Goffredo Baroncelli]

On 02/12/2013 02:25 AM, Tsutomu Itoh wrote:
> Currently, the following commands succeed.
> 
>  # cat /proc/swaps
>  Filename                                Type            Size    Used    Priority
>  /dev/sda3                               partition       8388604 0       -1
>  /dev/sdc8                               partition       9765884 0       -2
>  # mkfs.btrfs /dev/sdc8
>  
>  WARNING! - Btrfs v0.20-rc1-165-g82ac345 IS EXPERIMENTAL
>  WARNING! - see http://btrfs.wiki.kernel.org before using
>  
>  fs created label (null) on /dev/sdc8
>          nodesize 4096 leafsize 4096 sectorsize 4096 size 9.31GB
>  Btrfs v0.20-rc1-165-g82ac345
>  # btrfs fi sh /dev/sdc8
>  Label: none  uuid: fc0bdbd0-7eed-460f-b4e9-131273b66df2
>          Total devices 1 FS bytes used 28.00KB
>          devid    1 size 9.31GB used 989.62MB path /dev/sdc8
>  
>  Btrfs v0.20-rc1-165-g82ac345
>  #
> 
> But we should check out the swap device. So fixed it.



> 
> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
> ---
> (this patch is based on Chris's raid56-experimental branch)
> ---
>  mkfs.c  | 18 ++++++++++++++++++
>  utils.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  utils.h |  1 +
>  3 files changed, 68 insertions(+)
> 
> diff --git a/mkfs.c b/mkfs.c
> index 2d3c2af..fdc3373 100644
> --- a/mkfs.c
> +++ b/mkfs.c
> @@ -1366,6 +1366,15 @@ int main(int ac, char **av)
>  
>  	if (source_dir == 0) {
>  		file = av[optind++];
> +		ret = is_swap_device(file);
> +		if (ret < 0) {
> +			fprintf(stderr, "error checking %s status\n", file);
> +			exit(1);
> +		}

The fact that it is not possible to perform a check shouldn't prohibit
to run a mkfs.btrfs.

It is possible to add a switch to bypass this kind of checks ? We should
allow the user to be not limited by the fact that the check fails. I am
thinking to a "rescue" scenario like boot in a single mode where not al
filesystem are mounted.

I am referring to all the "safety" check not this one only.

BR
G.Baroncelli

[...]
-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5