Re: [PATCH] Btrfs: stop using vfs_read in send

2013-10-29 Thread Zach Brown
Sorry for the latency.  I was away last week.

On Fri, Oct 25, 2013 at 11:37:53AM -0400, Josef Bacik wrote:
> Apparently we don't actually close the files until we return to userspace, so
> stop using vfs_read in send.  This is actually better for us since we can 
> avoid
> all the extra logic of holding the file we're sending open and making sure to
> clean it up.

Good stuff.

A few things:

> + pgoff_t index = offset >> PAGE_CACHE_SHIFT;
> + pgoff_t last_index;
> + unsigned pg_offset = offset & (PAGE_CACHE_SIZE - 1);

unsigned pg_offset = offset & ~PAGE_CACHE_MASK;

> + if (offset + len > i_size_read(inode)) {

I worry about offset + len wrapping but I didn't check the caller to see
if that's possible.

> + while (index <= last_index) {
> + unsigned cur_len = min_t(unsigned, len, PAGE_CACHE_SIZE);

Careful, this'd insert some random kernel memory after the first partial
page.

unsigned cur_len = min_t(unsigned, len, PAGE_CACHE_SIZE - 
pg_off);

> + addr = kmap(page);
> + memcpy(sctx->read_buf + ret, addr + pg_offset, cur_len);
> + kunmap(page);
> + unlock_page(page);

I think you need flush_dcache_page() before reading from user mappable
page cache pages.  (Need to make sure that potential cached stores at
other user virtual addreses are flushed before we read from the kernel
mapping.)

- z
--
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] Btrfs: stop using vfs_read in send

2013-10-25 Thread Josef Bacik
Apparently we don't actually close the files until we return to userspace, so
stop using vfs_read in send.  This is actually better for us since we can avoid
all the extra logic of holding the file we're sending open and making sure to
clean it up.  This will fix people who have been hitting too many files open
errors when trying to send.  Thanks,

Signed-off-by: Josef Bacik 
---
 fs/btrfs/send.c | 181 +++-
 1 file changed, 73 insertions(+), 108 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index eb4b458..2f43623 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -122,7 +122,6 @@ struct send_ctx {
struct list_head name_cache_list;
int name_cache_size;
 
-   struct file *cur_inode_filp;
char *read_buf;
 };
 
@@ -2128,77 +2127,6 @@ out:
 }
 
 /*
- * Called for regular files when sending extents data. Opens a struct file
- * to read from the file.
- */
-static int open_cur_inode_file(struct send_ctx *sctx)
-{
-   int ret = 0;
-   struct btrfs_key key;
-   struct path path;
-   struct inode *inode;
-   struct dentry *dentry;
-   struct file *filp;
-   int new = 0;
-
-   if (sctx->cur_inode_filp)
-   goto out;
-
-   key.objectid = sctx->cur_ino;
-   key.type = BTRFS_INODE_ITEM_KEY;
-   key.offset = 0;
-
-   inode = btrfs_iget(sctx->send_root->fs_info->sb, &key, sctx->send_root,
-   &new);
-   if (IS_ERR(inode)) {
-   ret = PTR_ERR(inode);
-   goto out;
-   }
-
-   dentry = d_obtain_alias(inode);
-   inode = NULL;
-   if (IS_ERR(dentry)) {
-   ret = PTR_ERR(dentry);
-   goto out;
-   }
-
-   path.mnt = sctx->mnt;
-   path.dentry = dentry;
-   filp = dentry_open(&path, O_RDONLY | O_LARGEFILE, current_cred());
-   dput(dentry);
-   dentry = NULL;
-   if (IS_ERR(filp)) {
-   ret = PTR_ERR(filp);
-   goto out;
-   }
-   sctx->cur_inode_filp = filp;
-
-out:
-   /*
-* no xxxput required here as every vfs op
-* does it by itself on failure
-*/
-   return ret;
-}
-
-/*
- * Closes the struct file that was created in open_cur_inode_file
- */
-static int close_cur_inode_file(struct send_ctx *sctx)
-{
-   int ret = 0;
-
-   if (!sctx->cur_inode_filp)
-   goto out;
-
-   ret = filp_close(sctx->cur_inode_filp, NULL);
-   sctx->cur_inode_filp = NULL;
-
-out:
-   return ret;
-}
-
-/*
  * Sends a BTRFS_SEND_C_SUBVOL command/item to userspace
  */
 static int send_subvol_begin(struct send_ctx *sctx)
@@ -3630,6 +3558,72 @@ out:
return ret;
 }
 
+static ssize_t fill_read_buf(struct send_ctx *sctx, u64 offset, u32 len)
+{
+   struct btrfs_root *root = sctx->send_root;
+   struct btrfs_fs_info *fs_info = root->fs_info;
+   struct inode *inode;
+   struct page *page;
+   char *addr;
+   struct btrfs_key key;
+   pgoff_t index = offset >> PAGE_CACHE_SHIFT;
+   pgoff_t last_index;
+   unsigned pg_offset = offset & (PAGE_CACHE_SIZE - 1);
+   ssize_t ret = 0;
+
+   key.objectid = sctx->cur_ino;
+   key.type = BTRFS_INODE_ITEM_KEY;
+   key.offset = 0;
+
+   inode = btrfs_iget(fs_info->sb, &key, root, NULL);
+   if (IS_ERR(inode))
+   return PTR_ERR(inode);
+
+   if (offset + len > i_size_read(inode)) {
+   if (offset > i_size_read(inode))
+   len = 0;
+   else
+   len = offset - i_size_read(inode);
+   }
+   if (len == 0)
+   goto out;
+
+   last_index = (offset + len - 1) >> PAGE_CACHE_SHIFT;
+   while (index <= last_index) {
+   unsigned cur_len = min_t(unsigned, len, PAGE_CACHE_SIZE);
+
+   page = find_or_create_page(inode->i_mapping, index, GFP_NOFS);
+   if (!page) {
+   ret = -ENOMEM;
+   break;
+   }
+
+   if (!PageUptodate(page)) {
+   btrfs_readpage(NULL, page);
+   lock_page(page);
+   if (!PageUptodate(page)) {
+   unlock_page(page);
+   page_cache_release(page);
+   ret = -EIO;
+   break;
+   }
+   }
+
+   addr = kmap(page);
+   memcpy(sctx->read_buf + ret, addr + pg_offset, cur_len);
+   kunmap(page);
+   unlock_page(page);
+   page_cache_release(page);
+   index++;
+   pg_offset = 0;
+   len -= cur_len;
+   ret += cur_len;
+   }
+out:
+   iput(inode);
+   return ret;
+}
+
 /*
  * Read some bytes from the current inode/file and send a write command to
  * user space.
@@ -3638