[RFC] odd thing in btrfs_file_aio_write()

[RFC] odd thing in btrfs_file_aio_write()

[Al Viro]

		end_pos = round_up(pos + iov->iov_len, root->sectorsize);

added in commit 3ac0d7b96a268a98bd474cab8bce3a9f125aaccf
Author: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date:   Thu Mar 27 02:51:58 2014 +0000

    btrfs: Change the expanding write sequence to fix snapshot related bug.

doesn't look right - after all, just split the first iovec in the array
and end_pos will go down.  Do we want pos + count instead of
pos + iov->iov_len there?

Re: [RFC] odd thing in btrfs_file_aio_write()

[Qu Wenruo]

Oh, that's my fault, I forgot that iov can be chained.

I should use 'pos + count' instead.

Thanks for pointing it out.
Qu.

-------- Original Message --------
Subject: [RFC] odd thing in btrfs_file_aio_write()
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Chris Mason <clm@fb.com>
Date: 2014年04月14日 08:26
> 		end_pos = round_up(pos + iov->iov_len, root->sectorsize);
>
> added in commit 3ac0d7b96a268a98bd474cab8bce3a9f125aaccf
> Author: Qu Wenruo <quwenruo@cn.fujitsu.com>
> Date:   Thu Mar 27 02:51:58 2014 +0000
>
>      btrfs: Change the expanding write sequence to fix snapshot related bug.
>
> doesn't look right - after all, just split the first iovec in the array
> and end_pos will go down.  Do we want pos + count instead of
> pos + iov->iov_len there?

Re: [RFC] odd thing in btrfs_file_aio_write()

[Qu Wenruo]

Oh, that's my fault, I forgot that iov can be chained.

I should use 'pos + count' instead.

Thanks for pointing it out.
Qu.

-------- Original Message --------
Subject: [RFC] odd thing in btrfs_file_aio_write()
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Chris Mason <clm@fb.com>
Date: 2014年04月14日 08:26
> 		end_pos = round_up(pos + iov->iov_len, root->sectorsize);
>
> added in commit 3ac0d7b96a268a98bd474cab8bce3a9f125aaccf
> Author: Qu Wenruo <quwenruo@cn.fujitsu.com>
> Date:   Thu Mar 27 02:51:58 2014 +0000
>
>      btrfs: Change the expanding write sequence to fix snapshot related bug.
>
> doesn't look right - after all, just split the first iovec in the array
> and end_pos will go down.  Do we want pos + count instead of
> pos + iov->iov_len there?

--
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: [RFC] odd thing in btrfs_file_aio_write()

[Al Viro]

On Mon, Apr 14, 2014 at 10:35:08AM +0800, Qu Wenruo wrote:
> Oh, that's my fault, I forgot that iov can be chained.
> 
> I should use 'pos + count' instead.

BTW, will there be any difference if 10Mb write starts one byte before EOF?
IOW, is that if (start_pos > i_size_read(inode)) { in there correct
these days?  And what'll happen if we hit e.g. an unmapped page in the
middle of the data being written?  That will result in short write, but
will it truncate what's left of that dummy range?

Re: [RFC] odd thing in btrfs_file_aio_write()

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [RFC] odd thing in btrfs_file_aio_write()
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年04月14日 10:48
> On Mon, Apr 14, 2014 at 10:35:08AM +0800, Qu Wenruo wrote:
>> Oh, that's my fault, I forgot that iov can be chained.
>>
>> I should use 'pos + count' instead.
> BTW, will there be any difference if 10Mb write starts one byte before EOF?
This will not be a problem, since btrfs will not update the inode size 
until the data is written.
So this behavior will not cause empty gap.
> IOW, is that if (start_pos > i_size_read(inode)) { in there correct
> these days?  And what'll happen if we hit e.g. an unmapped page in the
> middle of the data being written?  That will result in short write, but
> will it truncate what's left of that dummy range?
I'm very sorry for my poor that I could not understand the question well.
Would you please explain what does the "unmapped page" means?

Did you mean two noncontinuous iovecs?
If you did mean that, it seems that I should expand the end_pos to the 
end of the iovec...

Thanks,
Qu

Re: [RFC] odd thing in btrfs_file_aio_write()

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [RFC] odd thing in btrfs_file_aio_write()
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年04月14日 10:48
> On Mon, Apr 14, 2014 at 10:35:08AM +0800, Qu Wenruo wrote:
>> Oh, that's my fault, I forgot that iov can be chained.
>>
>> I should use 'pos + count' instead.
> BTW, will there be any difference if 10Mb write starts one byte before EOF?
This will not be a problem, since btrfs will not update the inode size 
until the data is written.
So this behavior will not cause empty gap.
> IOW, is that if (start_pos > i_size_read(inode)) { in there correct
> these days?  And what'll happen if we hit e.g. an unmapped page in the
> middle of the data being written?  That will result in short write, but
> will it truncate what's left of that dummy range?
I'm very sorry for my poor that I could not understand the question well.
Would you please explain what does the "unmapped page" means?

Did you mean two noncontinuous iovecs?
If you did mean that, it seems that I should expand the end_pos to the 
end of the iovec...

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

Re: [RFC] odd thing in btrfs_file_aio_write()

[Al Viro]

On Mon, Apr 14, 2014 at 11:36:53AM +0800, Qu Wenruo wrote:
> >IOW, is that if (start_pos > i_size_read(inode)) { in there correct
> >these days?  And what'll happen if we hit e.g. an unmapped page in the
> >middle of the data being written?  That will result in short write, but
> >will it truncate what's left of that dummy range?
> I'm very sorry for my poor that I could not understand the question well.
> Would you please explain what does the "unmapped page" means?
> 
> Did you mean two noncontinuous iovecs?
> If you did mean that, it seems that I should expand the end_pos to
> the end of the iovec...

I mean that the very first (and only) iovec can very well span an area
that has been munmapped():

	char *buf = (char *)mmap(NULL, 65536, PROT_READ, MAP_ANON, -1, 0);
	memset(buf, 'A', 65536);
	munmap(buf + 4096, 4096);
	write(fd, buf, 65536);

or

	char *buf = (char *)mmap(NULL, 65536, PROT_READ, MAP_ANON, -1, 0);
	struct iovec iv = {buf, 65536};
	memset(buf, 'A', 65536);
	munmap(buf + 4096, 4096);
	writev(fd, &iv, 1);

will end up writing 4Kb of data (filled with 'A') and return 4096.  That's
how short writes happen...

Re: [RFC] odd thing in btrfs_file_aio_write()

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [RFC] odd thing in btrfs_file_aio_write()
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年04月14日 13:08
> On Mon, Apr 14, 2014 at 11:36:53AM +0800, Qu Wenruo wrote:
>>> IOW, is that if (start_pos > i_size_read(inode)) { in there correct
>>> these days?  And what'll happen if we hit e.g. an unmapped page in the
>>> middle of the data being written?  That will result in short write, but
>>> will it truncate what's left of that dummy range?
>> I'm very sorry for my poor that I could not understand the question well.
>> Would you please explain what does the "unmapped page" means?
>>
>> Did you mean two noncontinuous iovecs?
>> If you did mean that, it seems that I should expand the end_pos to
>> the end of the iovec...
> I mean that the very first (and only) iovec can very well span an area
> that has been munmapped():
>
> 	char *buf = (char *)mmap(NULL, 65536, PROT_READ, MAP_ANON, -1, 0);
> 	memset(buf, 'A', 65536);
> 	munmap(buf + 4096, 4096);
> 	write(fd, buf, 65536);
>
> or
>
> 	char *buf = (char *)mmap(NULL, 65536, PROT_READ, MAP_ANON, -1, 0);
> 	struct iovec iv = {buf, 65536};
> 	memset(buf, 'A', 65536);
> 	munmap(buf + 4096, 4096);
> 	writev(fd, &iv, 1);
>
> will end up writing 4Kb of data (filled with 'A') and return 4096.  That's
> how short writes happen...
Since the inode size on disk is updated only when the write finished, so 
even the hole punched is larger
than the data written, it will be OK since the inode size on disk is not 
expended to 64K.

Although short writes is OK, when two noncontinuous iovecs is passed, 
there is still empty gap,
so I still needs to fix the code.

Thanks,
Qu

Re: [RFC] odd thing in btrfs_file_aio_write()

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [RFC] odd thing in btrfs_file_aio_write()
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年04月14日 13:08
> On Mon, Apr 14, 2014 at 11:36:53AM +0800, Qu Wenruo wrote:
>>> IOW, is that if (start_pos > i_size_read(inode)) { in there correct
>>> these days?  And what'll happen if we hit e.g. an unmapped page in the
>>> middle of the data being written?  That will result in short write, but
>>> will it truncate what's left of that dummy range?
>> I'm very sorry for my poor that I could not understand the question well.
>> Would you please explain what does the "unmapped page" means?
>>
>> Did you mean two noncontinuous iovecs?
>> If you did mean that, it seems that I should expand the end_pos to
>> the end of the iovec...
> I mean that the very first (and only) iovec can very well span an area
> that has been munmapped():
>
> 	char *buf = (char *)mmap(NULL, 65536, PROT_READ, MAP_ANON, -1, 0);
> 	memset(buf, 'A', 65536);
> 	munmap(buf + 4096, 4096);
> 	write(fd, buf, 65536);
>
> or
>
> 	char *buf = (char *)mmap(NULL, 65536, PROT_READ, MAP_ANON, -1, 0);
> 	struct iovec iv = {buf, 65536};
> 	memset(buf, 'A', 65536);
> 	munmap(buf + 4096, 4096);
> 	writev(fd, &iv, 1);
>
> will end up writing 4Kb of data (filled with 'A') and return 4096.  That's
> how short writes happen...
Since the inode size on disk is updated only when the write finished, so 
even the hole punched is larger
than the data written, it will be OK since the inode size on disk is not 
expended to 64K.

Although short writes is OK, when two noncontinuous iovecs is passed, 
there is still empty gap,
so I still needs to fix the code.

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