[PATCH] f2fs-tools: avoid mounting f2fs if tools already open the device

[PATCH] f2fs-tools: avoid mounting f2fs if tools already open the device

[Sheng Yong]

If the block device is opened by tools, F2FS should not be mounted.
Especially when fsck is running, errors unexpected may happen. So if
tools open a block device, we give it the O_EXCL flag to make sure
the block device is opened exclusivly.

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 lib/libf2fs.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index b25fbf2..5625cff 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -783,21 +783,35 @@ int get_device_info(int i)
 #endif
 	struct device_info *dev = c.devices + i;
 
+	stat_buf = malloc(sizeof(struct stat));
+	ASSERT(stat_buf);
+	if (stat(dev->path, stat_buf) < 0 ) {
+		MSG(0, "\tError: Failed to get the device stat!\n");
+		free(stat_buf);
+		return -1;
+	}
+
 	if (c.sparse_mode) {
-		fd = open((char *)dev->path, O_RDWR | O_CREAT | O_BINARY, 0644);
+		fd = open(dev->path, O_RDWR | O_CREAT | O_BINARY, 0644);
 	} else {
-		fd = open((char *)dev->path, O_RDWR);
+		if (S_ISBLK(stat_buf->st_mode))
+			fd = open(dev->path, O_RDWR | O_EXCL);
+		else
+			fd = open(dev->path, O_RDWR);
 	}
 	if (fd < 0) {
 		MSG(0, "\tError: Failed to open the device!\n");
+		free(stat_buf);
 		return -1;
 	}
 
 	dev->fd = fd;
 
 	if (c.sparse_mode) {
-		if (f2fs_init_sparse_file())
+		if (f2fs_init_sparse_file()) {
+			free(stat_buf);
 			return -1;
+		}
 	}
 
 	if (c.kd == -1) {
@@ -810,13 +824,6 @@ int get_device_info(int i)
 		}
 	}
 
-	stat_buf = malloc(sizeof(struct stat));
-	if (fstat(fd, stat_buf) < 0 ) {
-		MSG(0, "\tError: Failed to get the device stat!\n");
-		free(stat_buf);
-		return -1;
-	}
-
 	if (c.sparse_mode) {
 		dev->total_sectors = c.device_size / dev->sector_size;
 	} else if (S_ISREG(stat_buf->st_mode)) {
-- 
2.17.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs-tools: avoid mounting f2fs if tools already open the device

[Chao Yu]

On 2018/7/4 17:50, Sheng Yong wrote:
> If the block device is opened by tools, F2FS should not be mounted.
> Especially when fsck is running, errors unexpected may happen. So if
> tools open a block device, we give it the O_EXCL flag to make sure
> the block device is opened exclusivly.

Should we treat block inode and regular inode as the same?

Not sure about this, since for debuging, if we can't umount due to panic in
f2fs, still we can dump last data in device.

Thanks,

> 
> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
> ---
>  lib/libf2fs.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
> index b25fbf2..5625cff 100644
> --- a/lib/libf2fs.c
> +++ b/lib/libf2fs.c
> @@ -783,21 +783,35 @@ int get_device_info(int i)
>  #endif
>  	struct device_info *dev = c.devices + i;
>  
> +	stat_buf = malloc(sizeof(struct stat));
> +	ASSERT(stat_buf);
> +	if (stat(dev->path, stat_buf) < 0 ) {
> +		MSG(0, "\tError: Failed to get the device stat!\n");
> +		free(stat_buf);
> +		return -1;
> +	}
> +
>  	if (c.sparse_mode) {
> -		fd = open((char *)dev->path, O_RDWR | O_CREAT | O_BINARY, 0644);
> +		fd = open(dev->path, O_RDWR | O_CREAT | O_BINARY, 0644);
>  	} else {
> -		fd = open((char *)dev->path, O_RDWR);
> +		if (S_ISBLK(stat_buf->st_mode))
> +			fd = open(dev->path, O_RDWR | O_EXCL);
> +		else
> +			fd = open(dev->path, O_RDWR);
>  	}
>  	if (fd < 0) {
>  		MSG(0, "\tError: Failed to open the device!\n");
> +		free(stat_buf);
>  		return -1;
>  	}
>  
>  	dev->fd = fd;
>  
>  	if (c.sparse_mode) {
> -		if (f2fs_init_sparse_file())
> +		if (f2fs_init_sparse_file()) {
> +			free(stat_buf);
>  			return -1;
> +		}
>  	}
>  
>  	if (c.kd == -1) {
> @@ -810,13 +824,6 @@ int get_device_info(int i)
>  		}
>  	}
>  
> -	stat_buf = malloc(sizeof(struct stat));
> -	if (fstat(fd, stat_buf) < 0 ) {
> -		MSG(0, "\tError: Failed to get the device stat!\n");
> -		free(stat_buf);
> -		return -1;
> -	}
> -
>  	if (c.sparse_mode) {
>  		dev->total_sectors = c.device_size / dev->sector_size;
>  	} else if (S_ISREG(stat_buf->st_mode)) {
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs-tools: avoid mounting f2fs if tools already open the device

[Sheng Yong]

Hi, Chao,

On 2018/7/9 23:14, Chao Yu wrote:
> On 2018/7/4 17:50, Sheng Yong wrote:
>> If the block device is opened by tools, F2FS should not be mounted.
>> Especially when fsck is running, errors unexpected may happen. So if
>> tools open a block device, we give it the O_EXCL flag to make sure
>> the block device is opened exclusivly.
> 
> Should we treat block inode and regular inode as the same?

I'm afraid not, the behavior of O_EXCL is not defined for regular inode
if it is used without O_CREAT. If f2fs is installed in a image file (not
a block device), we may still find a another way to avoid fsck and f2fs
run concurrently :(

thanks
> 
> Not sure about this, since for debuging, if we can't umount due to panic in
> f2fs, still we can dump last data in device.
> 
> Thanks,
> 
>>
>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>> ---
>>   lib/libf2fs.c | 27 +++++++++++++++++----------
>>   1 file changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
>> index b25fbf2..5625cff 100644
>> --- a/lib/libf2fs.c
>> +++ b/lib/libf2fs.c
>> @@ -783,21 +783,35 @@ int get_device_info(int i)
>>   #endif
>>   	struct device_info *dev = c.devices + i;
>>   
>> +	stat_buf = malloc(sizeof(struct stat));
>> +	ASSERT(stat_buf);
>> +	if (stat(dev->path, stat_buf) < 0 ) {
>> +		MSG(0, "\tError: Failed to get the device stat!\n");
>> +		free(stat_buf);
>> +		return -1;
>> +	}
>> +
>>   	if (c.sparse_mode) {
>> -		fd = open((char *)dev->path, O_RDWR | O_CREAT | O_BINARY, 0644);
>> +		fd = open(dev->path, O_RDWR | O_CREAT | O_BINARY, 0644);
>>   	} else {
>> -		fd = open((char *)dev->path, O_RDWR);
>> +		if (S_ISBLK(stat_buf->st_mode))
>> +			fd = open(dev->path, O_RDWR | O_EXCL);
>> +		else
>> +			fd = open(dev->path, O_RDWR);
>>   	}
>>   	if (fd < 0) {
>>   		MSG(0, "\tError: Failed to open the device!\n");
>> +		free(stat_buf);
>>   		return -1;
>>   	}
>>   
>>   	dev->fd = fd;
>>   
>>   	if (c.sparse_mode) {
>> -		if (f2fs_init_sparse_file())
>> +		if (f2fs_init_sparse_file()) {
>> +			free(stat_buf);
>>   			return -1;
>> +		}
>>   	}
>>   
>>   	if (c.kd == -1) {
>> @@ -810,13 +824,6 @@ int get_device_info(int i)
>>   		}
>>   	}
>>   
>> -	stat_buf = malloc(sizeof(struct stat));
>> -	if (fstat(fd, stat_buf) < 0 ) {
>> -		MSG(0, "\tError: Failed to get the device stat!\n");
>> -		free(stat_buf);
>> -		return -1;
>> -	}
>> -
>>   	if (c.sparse_mode) {
>>   		dev->total_sectors = c.device_size / dev->sector_size;
>>   	} else if (S_ISREG(stat_buf->st_mode)) {
>>
> 
> .
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs-tools: avoid mounting f2fs if tools already open the device

[Chao Yu]

Hi Sheng,

On 2018/7/10 9:53, Sheng Yong wrote:
> Hi, Chao,
> 
> On 2018/7/9 23:14, Chao Yu wrote:
>> On 2018/7/4 17:50, Sheng Yong wrote:
>>> If the block device is opened by tools, F2FS should not be mounted.
>>> Especially when fsck is running, errors unexpected may happen. So if
>>> tools open a block device, we give it the O_EXCL flag to make sure
>>> the block device is opened exclusivly.
>>
>> Should we treat block inode and regular inode as the same?
> 
> I'm afraid not, the behavior of O_EXCL is not defined for regular inode
> if it is used without O_CREAT. If f2fs is installed in a image file (not
> a block device), we may still find a another way to avoid fsck and f2fs
> run concurrently :(

Correct, I misunderstand semantics of this flag.

> 
> thanks
>>
>> Not sure about this, since for debuging, if we can't umount due to panic in
>> f2fs, still we can dump last data in device.
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

>>> ---
>>>   lib/libf2fs.c | 27 +++++++++++++++++----------
>>>   1 file changed, 17 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
>>> index b25fbf2..5625cff 100644
>>> --- a/lib/libf2fs.c
>>> +++ b/lib/libf2fs.c
>>> @@ -783,21 +783,35 @@ int get_device_info(int i)
>>>   #endif
>>>   	struct device_info *dev = c.devices + i;
>>>   
>>> +	stat_buf = malloc(sizeof(struct stat));
>>> +	ASSERT(stat_buf);
>>> +	if (stat(dev->path, stat_buf) < 0 ) {
>>> +		MSG(0, "\tError: Failed to get the device stat!\n");
>>> +		free(stat_buf);
>>> +		return -1;
>>> +	}
>>> +
>>>   	if (c.sparse_mode) {
>>> -		fd = open((char *)dev->path, O_RDWR | O_CREAT | O_BINARY, 0644);
>>> +		fd = open(dev->path, O_RDWR | O_CREAT | O_BINARY, 0644);
>>>   	} else {
>>> -		fd = open((char *)dev->path, O_RDWR);
>>> +		if (S_ISBLK(stat_buf->st_mode))
>>> +			fd = open(dev->path, O_RDWR | O_EXCL);
>>> +		else
>>> +			fd = open(dev->path, O_RDWR);
>>>   	}
>>>   	if (fd < 0) {
>>>   		MSG(0, "\tError: Failed to open the device!\n");
>>> +		free(stat_buf);
>>>   		return -1;
>>>   	}
>>>   
>>>   	dev->fd = fd;
>>>   
>>>   	if (c.sparse_mode) {
>>> -		if (f2fs_init_sparse_file())
>>> +		if (f2fs_init_sparse_file()) {
>>> +			free(stat_buf);
>>>   			return -1;
>>> +		}
>>>   	}
>>>   
>>>   	if (c.kd == -1) {
>>> @@ -810,13 +824,6 @@ int get_device_info(int i)
>>>   		}
>>>   	}
>>>   
>>> -	stat_buf = malloc(sizeof(struct stat));
>>> -	if (fstat(fd, stat_buf) < 0 ) {
>>> -		MSG(0, "\tError: Failed to get the device stat!\n");
>>> -		free(stat_buf);
>>> -		return -1;
>>> -	}
>>> -
>>>   	if (c.sparse_mode) {
>>>   		dev->total_sectors = c.device_size / dev->sector_size;
>>>   	} else if (S_ISREG(stat_buf->st_mode)) {
>>>
>>
>> .
>>
> 
> 
> .
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot