Re: [RFC PATCH 1/8] btrfs: use iocb for __btrfs_buffered_write

[David Sterba]

On Tue, May 22, 2018 at 03:40:18PM +0900, Misono Tomohiro wrote:
> > @@ -1815,7 +1816,6 @@ static ssize_t __btrfs_direct_write(struct kiocb 
> > *iocb, struct iov_iter *from)
> >  {
> > struct file *file = iocb->ki_filp;
> > struct inode *inode = file_inode(file);
> > -   loff_t pos = iocb->ki_pos;
> > ssize_t written;
> > ssize_t written_buffered;
> > loff_t endbyte;
> > @@ -1826,8 +1826,8 @@ static ssize_t __btrfs_direct_write(struct kiocb 
> > *iocb, struct iov_iter *from)
> > if (written < 0 || !iov_iter_count(from))
> > return written;
> >  
> > -   pos += written;
> > -   written_buffered = __btrfs_buffered_write(file, from, pos);
> 
> > +   iocb->ki_pos += written;
> 
> Hi,
> 
> I found btrfs/026 fails on current misc-next branch and
> git bisect points this commit.

Thanks, that's appreciated.

> I noticed generic_file_direct_write() already updates iocb->ki_pos, and 
> therefore
> above "iocb->ki_pos += written" is not needed.
> 
> > +   written_buffered = __btrfs_buffered_write(iocb, from);
> > if (written_buffered < 0) {
> > err = written_buffered;
> > goto out;
> > @@ -1836,16 +1836,16 @@ static ssize_t __btrfs_direct_write(struct kiocb 
> > *iocb, struct iov_iter *from)
> >  * Ensure all data is persisted. We want the next direct IO read to be
> >  * able to read what was just written.
> >  */
> > -   endbyte = pos + written_buffered - 1;
> > -   err = btrfs_fdatawrite_range(inode, pos, endbyte);
> > +   endbyte = iocb->ki_pos + written_buffered - 1;
> > +   err = btrfs_fdatawrite_range(inode, iocb->ki_pos, endbyte);
> > if (err)
> > goto out;
> > -   err = filemap_fdatawait_range(inode->i_mapping, pos, endbyte);
> > +   err = filemap_fdatawait_range(inode->i_mapping, iocb->ki_pos, endbyte);
> > if (err)
> > goto out;
> > +   iocb->ki_pos += written_buffered;
> > written += written_buffered;
> > -   iocb->ki_pos = pos + written_buffered;
> > -   invalidate_mapping_pages(file->f_mapping, pos >> PAGE_SHIFT,
> > +   invalidate_mapping_pages(file->f_mapping, iocb->ki_pos >> PAGE_SHIFT,
> >  endbyte >> PAGE_SHIFT);
> 
> Also, this invalidate_mapping_pages() should be done before updating 
> iocb->ki_pos
> to invalidate buffered written area.

This sounds like the patch needs more review, I treated it as more like
a cleanup so I'll drop it from misc-next and revisit once the whole
iomap patchset will be sent again.
--
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: [RFC PATCH 1/8] btrfs: use iocb for __btrfs_buffered_write

[Misono Tomohiro]

On 2017/11/18 2:44, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues 
> 
> Preparatory patch. It reduces the arguments to __btrfs_buffered_write
> to follow buffered_write() style.
> 
> Signed-off-by: Goldwyn Rodrigues 
> 
> ---
>  fs/btrfs/file.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index aafcc785f840..9bceb0e61361 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1572,10 +1572,11 @@ static noinline int check_can_nocow(struct 
> btrfs_inode *inode, loff_t pos,
>   return ret;
>  }
>  
> -static noinline ssize_t __btrfs_buffered_write(struct file *file,
> -struct iov_iter *i,
> -loff_t pos)
> +static noinline ssize_t __btrfs_buffered_write(struct kiocb *iocb,
> +struct iov_iter *i)
>  {
> + struct file *file = iocb->ki_filp;
> + loff_t pos = iocb->ki_pos;
>   struct inode *inode = file_inode(file);
>   struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>   struct btrfs_root *root = BTRFS_I(inode)->root;
> @@ -1815,7 +1816,6 @@ static ssize_t __btrfs_direct_write(struct kiocb *iocb, 
> struct iov_iter *from)
>  {
>   struct file *file = iocb->ki_filp;
>   struct inode *inode = file_inode(file);
> - loff_t pos = iocb->ki_pos;
>   ssize_t written;
>   ssize_t written_buffered;
>   loff_t endbyte;
> @@ -1826,8 +1826,8 @@ static ssize_t __btrfs_direct_write(struct kiocb *iocb, 
> struct iov_iter *from)
>   if (written < 0 || !iov_iter_count(from))
>   return written;
>  
> - pos += written;
> - written_buffered = __btrfs_buffered_write(file, from, pos);

> + iocb->ki_pos += written;

Hi,

I found btrfs/026 fails on current misc-next branch and
git bisect points this commit.

I noticed generic_file_direct_write() already updates iocb->ki_pos, and 
therefore
above "iocb->ki_pos += written" is not needed.

> + written_buffered = __btrfs_buffered_write(iocb, from);
>   if (written_buffered < 0) {
>   err = written_buffered;
>   goto out;
> @@ -1836,16 +1836,16 @@ static ssize_t __btrfs_direct_write(struct kiocb 
> *iocb, struct iov_iter *from)
>* Ensure all data is persisted. We want the next direct IO read to be
>* able to read what was just written.
>*/
> - endbyte = pos + written_buffered - 1;
> - err = btrfs_fdatawrite_range(inode, pos, endbyte);
> + endbyte = iocb->ki_pos + written_buffered - 1;
> + err = btrfs_fdatawrite_range(inode, iocb->ki_pos, endbyte);
>   if (err)
>   goto out;
> - err = filemap_fdatawait_range(inode->i_mapping, pos, endbyte);
> + err = filemap_fdatawait_range(inode->i_mapping, iocb->ki_pos, endbyte);
>   if (err)
>   goto out;
> + iocb->ki_pos += written_buffered;
>   written += written_buffered;
> - iocb->ki_pos = pos + written_buffered;
> - invalidate_mapping_pages(file->f_mapping, pos >> PAGE_SHIFT,
> + invalidate_mapping_pages(file->f_mapping, iocb->ki_pos >> PAGE_SHIFT,
>endbyte >> PAGE_SHIFT);

Also, this invalidate_mapping_pages() should be done before updating 
iocb->ki_pos
to invalidate buffered written area.

Thanks,
Tomohiro Misono

>  out:
>   return written ? written : err;
> @@ -1964,7 +1964,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
>   if (iocb->ki_flags & IOCB_DIRECT) {
>   num_written = __btrfs_direct_write(iocb, from);
>   } else {
> - num_written = __btrfs_buffered_write(file, from, pos);
> + num_written = __btrfs_buffered_write(iocb, from);
>   if (num_written > 0)
>   iocb->ki_pos = pos + num_written;
>   if (clean_page)
> 

--
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: [RFC PATCH 1/8] btrfs: use iocb for __btrfs_buffered_write

[David Sterba]

On Fri, Nov 17, 2017 at 11:44:47AM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues 
> 
> Preparatory patch. It reduces the arguments to __btrfs_buffered_write
> to follow buffered_write() style.
> 
> Signed-off-by: Goldwyn Rodrigues 

I got pointed to this patch that it could be applied independently, so
I'm adding it to btrfs queue.
--
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

[RFC PATCH 1/8] btrfs: use iocb for __btrfs_buffered_write

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues 

Preparatory patch. It reduces the arguments to __btrfs_buffered_write
to follow buffered_write() style.

Signed-off-by: Goldwyn Rodrigues 

---
 fs/btrfs/file.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index aafcc785f840..9bceb0e61361 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1572,10 +1572,11 @@ static noinline int check_can_nocow(struct btrfs_inode 
*inode, loff_t pos,
return ret;
 }
 
-static noinline ssize_t __btrfs_buffered_write(struct file *file,
-  struct iov_iter *i,
-  loff_t pos)
+static noinline ssize_t __btrfs_buffered_write(struct kiocb *iocb,
+  struct iov_iter *i)
 {
+   struct file *file = iocb->ki_filp;
+   loff_t pos = iocb->ki_pos;
struct inode *inode = file_inode(file);
struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
struct btrfs_root *root = BTRFS_I(inode)->root;
@@ -1815,7 +1816,6 @@ static ssize_t __btrfs_direct_write(struct kiocb *iocb, 
struct iov_iter *from)
 {
struct file *file = iocb->ki_filp;
struct inode *inode = file_inode(file);
-   loff_t pos = iocb->ki_pos;
ssize_t written;
ssize_t written_buffered;
loff_t endbyte;
@@ -1826,8 +1826,8 @@ static ssize_t __btrfs_direct_write(struct kiocb *iocb, 
struct iov_iter *from)
if (written < 0 || !iov_iter_count(from))
return written;
 
-   pos += written;
-   written_buffered = __btrfs_buffered_write(file, from, pos);
+   iocb->ki_pos += written;
+   written_buffered = __btrfs_buffered_write(iocb, from);
if (written_buffered < 0) {
err = written_buffered;
goto out;
@@ -1836,16 +1836,16 @@ static ssize_t __btrfs_direct_write(struct kiocb *iocb, 
struct iov_iter *from)
 * Ensure all data is persisted. We want the next direct IO read to be
 * able to read what was just written.
 */
-   endbyte = pos + written_buffered - 1;
-   err = btrfs_fdatawrite_range(inode, pos, endbyte);
+   endbyte = iocb->ki_pos + written_buffered - 1;
+   err = btrfs_fdatawrite_range(inode, iocb->ki_pos, endbyte);
if (err)
goto out;
-   err = filemap_fdatawait_range(inode->i_mapping, pos, endbyte);
+   err = filemap_fdatawait_range(inode->i_mapping, iocb->ki_pos, endbyte);
if (err)
goto out;
+   iocb->ki_pos += written_buffered;
written += written_buffered;
-   iocb->ki_pos = pos + written_buffered;
-   invalidate_mapping_pages(file->f_mapping, pos >> PAGE_SHIFT,
+   invalidate_mapping_pages(file->f_mapping, iocb->ki_pos >> PAGE_SHIFT,
 endbyte >> PAGE_SHIFT);
 out:
return written ? written : err;
@@ -1964,7 +1964,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
if (iocb->ki_flags & IOCB_DIRECT) {
num_written = __btrfs_direct_write(iocb, from);
} else {
-   num_written = __btrfs_buffered_write(file, from, pos);
+   num_written = __btrfs_buffered_write(iocb, from);
if (num_written > 0)
iocb->ki_pos = pos + num_written;
if (clean_page)
-- 
2.14.2

--
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