Re: [PATCH 3/3] btrfs: extended inode refs
On Wed, August 08, 2012 at 20:55 (+0200), Mark Fasheh wrote:
> The iterate_irefs in backref.c is used to build path components from inode
> refs. This patch adds code to iterate extended refs as well.
>
> I had modify the callback function signature to abstract out some of the
> differences between ref structures. iref_to_path() also needed similar
> changes.
>
> Signed-off-by: Mark Fasheh
> ---
> fs/btrfs/backref.c | 134
> +++-
> fs/btrfs/backref.h |2 -
> 2 files changed, 112 insertions(+), 24 deletions(-)
>
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index 658e09c..4a01f7c 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -1194,11 +1194,10 @@ int btrfs_find_one_extref(struct btrfs_root *root,
> u64 inode_objectid,
> * value will be smaller than dest. callers must check this!
> */
> static char *iref_to_path(struct btrfs_root *fs_root, struct btrfs_path
> *path,
> - struct btrfs_inode_ref *iref,
> - struct extent_buffer *eb_in, u64 parent,
> - char *dest, u32 size)
> + u32 name_len, unsigned long name_off,
> + struct extent_buffer *eb_in, u64 parent,
> + char *dest, u32 size)
> {
> - u32 len;
> int slot;
> u64 next_inum;
> int ret;
> @@ -1206,17 +1205,17 @@ static char *iref_to_path(struct btrfs_root *fs_root,
> struct btrfs_path *path,
> struct extent_buffer *eb = eb_in;
> struct btrfs_key found_key;
> int leave_spinning = path->leave_spinning;
> + struct btrfs_inode_ref *iref;
>
> if (bytes_left >= 0)
> dest[bytes_left] = '\0';
>
> path->leave_spinning = 1;
> while (1) {
> - len = btrfs_inode_ref_name_len(eb, iref);
> - bytes_left -= len;
> + bytes_left -= name_len;
> if (bytes_left >= 0)
> read_extent_buffer(eb, dest + bytes_left,
> - (unsigned long)(iref + 1), len);
> +name_off, name_len);
> if (eb != eb_in) {
> btrfs_tree_read_unlock_blocking(eb);
> free_extent_buffer(eb);
> @@ -1226,6 +1225,7 @@ static char *iref_to_path(struct btrfs_root *fs_root,
> struct btrfs_path *path,
> ret = -ENOENT;
> if (ret)
> break;
> +
> next_inum = found_key.offset;
>
> /* regular exit ahead */
> @@ -1241,8 +1241,11 @@ static char *iref_to_path(struct btrfs_root *fs_root,
> struct btrfs_path *path,
> btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
> }
> btrfs_release_path(path);
> -
> iref = btrfs_item_ptr(eb, slot, struct btrfs_inode_ref);
> +
> + name_len = btrfs_inode_ref_name_len(eb, iref);
> + name_off = (unsigned long)(iref + 1);
> +
> parent = next_inum;
> --bytes_left;
> if (bytes_left >= 0)
> @@ -1531,9 +1534,12 @@ int iterate_inodes_from_logical(u64 logical, struct
> btrfs_fs_info *fs_info,
> return ret;
> }
>
> -static int iterate_irefs(u64 inum, struct btrfs_root *fs_root,
> - struct btrfs_path *path,
> - iterate_irefs_t *iterate, void *ctx)
> +typedef int (iterate_irefs_t)(u64 parent, u32 name_len, unsigned long
> name_off,
> + struct extent_buffer *eb, void *ctx);
> +
> +static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
> + struct btrfs_path *path,
> + iterate_irefs_t *iterate, void *ctx)
> {
> int ret = 0;
> int slot;
> @@ -1550,7 +1556,7 @@ static int iterate_irefs(u64 inum, struct btrfs_root
> *fs_root,
> while (!ret) {
> path->leave_spinning = 1;
> ret = inode_ref_info(inum, parent ? parent+1 : 0, fs_root, path,
> - &found_key);
> + &found_key);
> if (ret < 0)
> break;
> if (ret) {
> @@ -1578,7 +1584,8 @@ static int iterate_irefs(u64 inum, struct btrfs_root
> *fs_root,
>"tree %llu\n", cur,
>(unsigned long long)found_key.objectid,
>(unsigned long long)fs_root->objectid);
> - ret = iterate(parent, iref, eb, ctx);
> + ret = iterate(parent, name_len,
> + (unsigned long)(iref + 1), eb, ctx);
> if (ret)
> break;
> len = sizeof(*iref) + name_len;
> @@ -1593,12 +1600,98 @@ static int iterate_iref
[PATCH 3/3] btrfs: extended inode refs
The iterate_irefs in backref.c is used to build path components from inode
refs. This patch adds code to iterate extended refs as well.
I had modify the callback function signature to abstract out some of the
differences between ref structures. iref_to_path() also needed similar
changes.
Signed-off-by: Mark Fasheh
---
fs/btrfs/backref.c | 134 +++-
fs/btrfs/backref.h |2 -
2 files changed, 112 insertions(+), 24 deletions(-)
diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 658e09c..4a01f7c 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1194,11 +1194,10 @@ int btrfs_find_one_extref(struct btrfs_root *root, u64
inode_objectid,
* value will be smaller than dest. callers must check this!
*/
static char *iref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
- struct btrfs_inode_ref *iref,
- struct extent_buffer *eb_in, u64 parent,
- char *dest, u32 size)
+ u32 name_len, unsigned long name_off,
+ struct extent_buffer *eb_in, u64 parent,
+ char *dest, u32 size)
{
- u32 len;
int slot;
u64 next_inum;
int ret;
@@ -1206,17 +1205,17 @@ static char *iref_to_path(struct btrfs_root *fs_root,
struct btrfs_path *path,
struct extent_buffer *eb = eb_in;
struct btrfs_key found_key;
int leave_spinning = path->leave_spinning;
+ struct btrfs_inode_ref *iref;
if (bytes_left >= 0)
dest[bytes_left] = '\0';
path->leave_spinning = 1;
while (1) {
- len = btrfs_inode_ref_name_len(eb, iref);
- bytes_left -= len;
+ bytes_left -= name_len;
if (bytes_left >= 0)
read_extent_buffer(eb, dest + bytes_left,
- (unsigned long)(iref + 1), len);
+ name_off, name_len);
if (eb != eb_in) {
btrfs_tree_read_unlock_blocking(eb);
free_extent_buffer(eb);
@@ -1226,6 +1225,7 @@ static char *iref_to_path(struct btrfs_root *fs_root,
struct btrfs_path *path,
ret = -ENOENT;
if (ret)
break;
+
next_inum = found_key.offset;
/* regular exit ahead */
@@ -1241,8 +1241,11 @@ static char *iref_to_path(struct btrfs_root *fs_root,
struct btrfs_path *path,
btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
}
btrfs_release_path(path);
-
iref = btrfs_item_ptr(eb, slot, struct btrfs_inode_ref);
+
+ name_len = btrfs_inode_ref_name_len(eb, iref);
+ name_off = (unsigned long)(iref + 1);
+
parent = next_inum;
--bytes_left;
if (bytes_left >= 0)
@@ -1531,9 +1534,12 @@ int iterate_inodes_from_logical(u64 logical, struct
btrfs_fs_info *fs_info,
return ret;
}
-static int iterate_irefs(u64 inum, struct btrfs_root *fs_root,
- struct btrfs_path *path,
- iterate_irefs_t *iterate, void *ctx)
+typedef int (iterate_irefs_t)(u64 parent, u32 name_len, unsigned long name_off,
+ struct extent_buffer *eb, void *ctx);
+
+static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
+ struct btrfs_path *path,
+ iterate_irefs_t *iterate, void *ctx)
{
int ret = 0;
int slot;
@@ -1550,7 +1556,7 @@ static int iterate_irefs(u64 inum, struct btrfs_root
*fs_root,
while (!ret) {
path->leave_spinning = 1;
ret = inode_ref_info(inum, parent ? parent+1 : 0, fs_root, path,
- &found_key);
+&found_key);
if (ret < 0)
break;
if (ret) {
@@ -1578,7 +1584,8 @@ static int iterate_irefs(u64 inum, struct btrfs_root
*fs_root,
"tree %llu\n", cur,
(unsigned long long)found_key.objectid,
(unsigned long long)fs_root->objectid);
- ret = iterate(parent, iref, eb, ctx);
+ ret = iterate(parent, name_len,
+ (unsigned long)(iref + 1), eb, ctx);
if (ret)
break;
len = sizeof(*iref) + name_len;
@@ -1593,12 +1600,98 @@ static int iterate_irefs(u64 inum, struct btrfs_root
*fs_root,
return ret;
}
+static int iterate_inode_extrefs(u64 inum, struct btrfs_root *fs_root,
+
Re: [PATCH 3/3] btrfs: extended inode refs
On Fri, Jul 06, 2012 at 04:57:29PM +0200, Jan Schmidt wrote:
> On Mon, May 21, 2012 at 23:46 (+0200), Mark Fasheh wrote:
> > From: Mark Fasheh
> >
> > The iterate_irefs in backref.c is used to build path components from inode
> > refs. This patch adds code to iterate extended refs as well.
> >
> > I had modify the callback function signature to abstract out some of the
> > differences between ref structures. iref_to_path() also needed similar
> > changes.
> >
> > Signed-off-by: Mark Fasheh
> > ---
> > fs/btrfs/backref.c | 144
> > +++-
> > fs/btrfs/backref.h |2 -
> > 2 files changed, 119 insertions(+), 27 deletions(-)
> >
> > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> > index c97240a..d88fa49 100644
> > --- a/fs/btrfs/backref.c
> > +++ b/fs/btrfs/backref.c
> > @@ -22,6 +22,7 @@
> > #include "ulist.h"
> > #include "transaction.h"
> > #include "delayed-ref.h"
> > +#include "locking.h"
>
> This + line tells me it's not based on top of linux-3.4 or newer. I see that
> the
> changes made in between are now included in your patch set. It might have been
> better to rebase it before sending them. Anyway, that only makes review a bit
> harder, should affect applying the patches.
Yes, it's all based on Linux 3.3 (when I started). I can rebase of course
but have avoided it so far in order to keep a stable base upon which to test
/ fix. I can rebase however, especially if that makes life easier for Chris.
> > /*
> > * this structure records all encountered refs on the way up to the root
> > @@ -940,34 +941,35 @@ int btrfs_find_one_extref(struct btrfs_root *root,
> > u64 inode_objectid,
> > * value will be smaller than dest. callers must check this!
> > */
> > static char *iref_to_path(struct btrfs_root *fs_root, struct btrfs_path
> > *path,
> > - struct btrfs_inode_ref *iref,
> > - struct extent_buffer *eb_in, u64 parent,
> > - char *dest, u32 size)
> > + u32 name_len, unsigned long name_off,
> > + struct extent_buffer *eb_in, u64 parent,
> > + char *dest, u32 size)
> > {
> > - u32 len;
> > int slot;
> > u64 next_inum;
> > int ret;
> > s64 bytes_left = size - 1;
> > struct extent_buffer *eb = eb_in;
> > struct btrfs_key found_key;
> > + struct btrfs_inode_ref *iref;
> >
> > if (bytes_left >= 0)
> > dest[bytes_left] = '\0';
> >
> > while (1) {
> > - len = btrfs_inode_ref_name_len(eb, iref);
> > - bytes_left -= len;
> > + bytes_left -= name_len;
> > if (bytes_left >= 0)
> > read_extent_buffer(eb, dest + bytes_left,
> > - (unsigned long)(iref + 1), len);
> > + name_off, name_len);
> > if (eb != eb_in)
> > free_extent_buffer(eb);
> > +
> > ret = inode_ref_info(parent, 0, fs_root, path, &found_key);
> > if (ret > 0)
> > ret = -ENOENT;
> > if (ret)
> > break;
> > +
> > next_inum = found_key.offset;
> >
> > /* regular exit ahead */
> > @@ -980,8 +982,11 @@ static char *iref_to_path(struct btrfs_root *fs_root,
> > struct btrfs_path *path,
> > if (eb != eb_in)
> > atomic_inc(&eb->refs);
> > btrfs_release_path(path);
> > -
> > iref = btrfs_item_ptr(eb, slot, struct btrfs_inode_ref);
> > +
> > + name_len = btrfs_inode_ref_name_len(eb, iref);
> > + name_off = (unsigned long)(iref + 1);
> > +
> > parent = next_inum;
> > --bytes_left;
> > if (bytes_left >= 0)
> > @@ -1294,9 +1299,12 @@ int iterate_inodes_from_logical(u64 logical, struct
> > btrfs_fs_info *fs_info,
> > return ret;
> > }
> >
> > -static int iterate_irefs(u64 inum, struct btrfs_root *fs_root,
> > - struct btrfs_path *path,
> > - iterate_irefs_t *iterate, void *ctx)
> > +typedef int (iterate_irefs_t)(u64 parent, u32 name_len, unsigned long
> > name_off,
> > + struct extent_buffer *eb, void *ctx);
> > +
> > +static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
> > + struct btrfs_path *path,
> > + iterate_irefs_t *iterate, void *ctx)
> > {
> > int ret;
> > int slot;
> > @@ -1312,7 +1320,7 @@ static int iterate_irefs(u64 inum, struct btrfs_root
> > *fs_root,
> >
> > while (1) {
> > ret = inode_ref_info(inum, parent ? parent+1 : 0, fs_root, path,
> > - &found_key);
> > +&found_key);
> > if (ret < 0)
> > break;
> > if (ret) {
> > @@ -
Re: [PATCH 3/3] btrfs: extended inode refs
On Mon, May 21, 2012 at 23:46 (+0200), Mark Fasheh wrote:
> From: Mark Fasheh
>
> The iterate_irefs in backref.c is used to build path components from inode
> refs. This patch adds code to iterate extended refs as well.
>
> I had modify the callback function signature to abstract out some of the
> differences between ref structures. iref_to_path() also needed similar
> changes.
>
> Signed-off-by: Mark Fasheh
> ---
> fs/btrfs/backref.c | 144
> +++-
> fs/btrfs/backref.h |2 -
> 2 files changed, 119 insertions(+), 27 deletions(-)
>
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index c97240a..d88fa49 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -22,6 +22,7 @@
> #include "ulist.h"
> #include "transaction.h"
> #include "delayed-ref.h"
> +#include "locking.h"
This + line tells me it's not based on top of linux-3.4 or newer. I see that the
changes made in between are now included in your patch set. It might have been
better to rebase it before sending them. Anyway, that only makes review a bit
harder, should affect applying the patches.
>
> /*
> * this structure records all encountered refs on the way up to the root
> @@ -940,34 +941,35 @@ int btrfs_find_one_extref(struct btrfs_root *root, u64
> inode_objectid,
> * value will be smaller than dest. callers must check this!
> */
> static char *iref_to_path(struct btrfs_root *fs_root, struct btrfs_path
> *path,
> - struct btrfs_inode_ref *iref,
> - struct extent_buffer *eb_in, u64 parent,
> - char *dest, u32 size)
> + u32 name_len, unsigned long name_off,
> + struct extent_buffer *eb_in, u64 parent,
> + char *dest, u32 size)
> {
> - u32 len;
> int slot;
> u64 next_inum;
> int ret;
> s64 bytes_left = size - 1;
> struct extent_buffer *eb = eb_in;
> struct btrfs_key found_key;
> + struct btrfs_inode_ref *iref;
>
> if (bytes_left >= 0)
> dest[bytes_left] = '\0';
>
> while (1) {
> - len = btrfs_inode_ref_name_len(eb, iref);
> - bytes_left -= len;
> + bytes_left -= name_len;
> if (bytes_left >= 0)
> read_extent_buffer(eb, dest + bytes_left,
> - (unsigned long)(iref + 1), len);
> +name_off, name_len);
> if (eb != eb_in)
> free_extent_buffer(eb);
> +
> ret = inode_ref_info(parent, 0, fs_root, path, &found_key);
> if (ret > 0)
> ret = -ENOENT;
> if (ret)
> break;
> +
> next_inum = found_key.offset;
>
> /* regular exit ahead */
> @@ -980,8 +982,11 @@ static char *iref_to_path(struct btrfs_root *fs_root,
> struct btrfs_path *path,
> if (eb != eb_in)
> atomic_inc(&eb->refs);
> btrfs_release_path(path);
> -
> iref = btrfs_item_ptr(eb, slot, struct btrfs_inode_ref);
> +
> + name_len = btrfs_inode_ref_name_len(eb, iref);
> + name_off = (unsigned long)(iref + 1);
> +
> parent = next_inum;
> --bytes_left;
> if (bytes_left >= 0)
> @@ -1294,9 +1299,12 @@ int iterate_inodes_from_logical(u64 logical, struct
> btrfs_fs_info *fs_info,
> return ret;
> }
>
> -static int iterate_irefs(u64 inum, struct btrfs_root *fs_root,
> - struct btrfs_path *path,
> - iterate_irefs_t *iterate, void *ctx)
> +typedef int (iterate_irefs_t)(u64 parent, u32 name_len, unsigned long
> name_off,
> + struct extent_buffer *eb, void *ctx);
> +
> +static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
> + struct btrfs_path *path,
> + iterate_irefs_t *iterate, void *ctx)
> {
> int ret;
> int slot;
> @@ -1312,7 +1320,7 @@ static int iterate_irefs(u64 inum, struct btrfs_root
> *fs_root,
>
> while (1) {
> ret = inode_ref_info(inum, parent ? parent+1 : 0, fs_root, path,
> - &found_key);
> + &found_key);
> if (ret < 0)
> break;
> if (ret) {
> @@ -1326,8 +1334,11 @@ static int iterate_irefs(u64 inum, struct btrfs_root
> *fs_root,
> eb = path->nodes[0];
> /* make sure we can use eb after releasing the path */
> atomic_inc(&eb->refs);
> + btrfs_tree_read_lock(eb);
> + btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
> btrfs_release_path(path);
>
> +
I realized you like adding n
[PATCH 3/3] btrfs: extended inode refs
From: Mark Fasheh
The iterate_irefs in backref.c is used to build path components from inode
refs. This patch adds code to iterate extended refs as well.
I had modify the callback function signature to abstract out some of the
differences between ref structures. iref_to_path() also needed similar
changes.
Signed-off-by: Mark Fasheh
---
fs/btrfs/backref.c | 144 +++-
fs/btrfs/backref.h |2 -
2 files changed, 119 insertions(+), 27 deletions(-)
diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index c97240a..d88fa49 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -22,6 +22,7 @@
#include "ulist.h"
#include "transaction.h"
#include "delayed-ref.h"
+#include "locking.h"
/*
* this structure records all encountered refs on the way up to the root
@@ -940,34 +941,35 @@ int btrfs_find_one_extref(struct btrfs_root *root, u64
inode_objectid,
* value will be smaller than dest. callers must check this!
*/
static char *iref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
- struct btrfs_inode_ref *iref,
- struct extent_buffer *eb_in, u64 parent,
- char *dest, u32 size)
+ u32 name_len, unsigned long name_off,
+ struct extent_buffer *eb_in, u64 parent,
+ char *dest, u32 size)
{
- u32 len;
int slot;
u64 next_inum;
int ret;
s64 bytes_left = size - 1;
struct extent_buffer *eb = eb_in;
struct btrfs_key found_key;
+ struct btrfs_inode_ref *iref;
if (bytes_left >= 0)
dest[bytes_left] = '\0';
while (1) {
- len = btrfs_inode_ref_name_len(eb, iref);
- bytes_left -= len;
+ bytes_left -= name_len;
if (bytes_left >= 0)
read_extent_buffer(eb, dest + bytes_left,
- (unsigned long)(iref + 1), len);
+ name_off, name_len);
if (eb != eb_in)
free_extent_buffer(eb);
+
ret = inode_ref_info(parent, 0, fs_root, path, &found_key);
if (ret > 0)
ret = -ENOENT;
if (ret)
break;
+
next_inum = found_key.offset;
/* regular exit ahead */
@@ -980,8 +982,11 @@ static char *iref_to_path(struct btrfs_root *fs_root,
struct btrfs_path *path,
if (eb != eb_in)
atomic_inc(&eb->refs);
btrfs_release_path(path);
-
iref = btrfs_item_ptr(eb, slot, struct btrfs_inode_ref);
+
+ name_len = btrfs_inode_ref_name_len(eb, iref);
+ name_off = (unsigned long)(iref + 1);
+
parent = next_inum;
--bytes_left;
if (bytes_left >= 0)
@@ -1294,9 +1299,12 @@ int iterate_inodes_from_logical(u64 logical, struct
btrfs_fs_info *fs_info,
return ret;
}
-static int iterate_irefs(u64 inum, struct btrfs_root *fs_root,
- struct btrfs_path *path,
- iterate_irefs_t *iterate, void *ctx)
+typedef int (iterate_irefs_t)(u64 parent, u32 name_len, unsigned long name_off,
+ struct extent_buffer *eb, void *ctx);
+
+static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
+ struct btrfs_path *path,
+ iterate_irefs_t *iterate, void *ctx)
{
int ret;
int slot;
@@ -1312,7 +1320,7 @@ static int iterate_irefs(u64 inum, struct btrfs_root
*fs_root,
while (1) {
ret = inode_ref_info(inum, parent ? parent+1 : 0, fs_root, path,
- &found_key);
+&found_key);
if (ret < 0)
break;
if (ret) {
@@ -1326,8 +1334,11 @@ static int iterate_irefs(u64 inum, struct btrfs_root
*fs_root,
eb = path->nodes[0];
/* make sure we can use eb after releasing the path */
atomic_inc(&eb->refs);
+ btrfs_tree_read_lock(eb);
+ btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
btrfs_release_path(path);
+
item = btrfs_item_nr(eb, slot);
iref = btrfs_item_ptr(eb, slot, struct btrfs_inode_ref);
@@ -1338,15 +1349,81 @@ static int iterate_irefs(u64 inum, struct btrfs_root
*fs_root,
"tree %llu\n", cur,
(unsigned long long)found_key.objectid,
(unsigned long long)fs_root->objectid);
- ret = iterate(parent, iref, eb, ctx);
- if
Re: [PATCH 3/3] btrfs: extended inode refs
On Thu, May 10, 2012 at 10:23:28AM +0200, Jan Schmidt wrote:
> On Wed, May 09, 2012 at 19:02 (+0200), Chris Mason wrote:
> +
> +while (1) {
> +ret = btrfs_find_one_extref(fs_root, inum, offset,
> path, &iref2,
> +&offset);
> +if (ret < 0)
> +break;
> +if (ret) {
> +ret = found ? 0 : -ENOENT;
> +break;
> +}
> +++found;
> +
> +slot = path->slots[0];
> +eb = path->nodes[0];
> +/* make sure we can use eb after releasing the path */
> +atomic_inc(&eb->refs);
> >>>
> >>> You need a blocking read lock here, too. Grab it before releasing the
> >>> path.
> >
> > If you're calling btrfs_search_slot, it will give you a blocking lock
> > on the leaf. If you set path->leave_spinning before the call, you'll
> > have a spinning lock on the leaf.
> >
> > If you unlock a block that you got from a path (like eb =
> > path->nodes[0]), the path structure has a flag for each level that
> > indicates if that block was locked or not. See btrfs_release_path().
> > So, don't fiddle the locks without fiddling the paths.
> >
> > You can switch from spinning to/from blocking without touching the path, it
> > figures that out.
>
> Note that we're releasing the path shortly after. My suggestion was to
> grab an *additional* read lock after atomic_inc(&eb->refs) (probably
> better extent_buffer_get(eb)) and before btrfs_path_release().
>
> An alternative would be to set path->locks[0] to NULL, which would
> btrfs_release_path prevent from unlocking it, kidnapping its lock for
> our purpose. But I much prefer the open coded solution.
Thanks, I see now.
-chris
--
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: [PATCH 3/3] btrfs: extended inode refs
On Wed, May 09, 2012 at 19:02 (+0200), Chris Mason wrote:
+
+ while (1) {
+ ret = btrfs_find_one_extref(fs_root, inum, offset, path, &iref2,
+ &offset);
+ if (ret < 0)
+ break;
+ if (ret) {
+ ret = found ? 0 : -ENOENT;
+ break;
+ }
+ ++found;
+
+ slot = path->slots[0];
+ eb = path->nodes[0];
+ /* make sure we can use eb after releasing the path */
+ atomic_inc(&eb->refs);
>>>
>>> You need a blocking read lock here, too. Grab it before releasing the path.
>
> If you're calling btrfs_search_slot, it will give you a blocking lock
> on the leaf. If you set path->leave_spinning before the call, you'll
> have a spinning lock on the leaf.
>
> If you unlock a block that you got from a path (like eb =
> path->nodes[0]), the path structure has a flag for each level that
> indicates if that block was locked or not. See btrfs_release_path().
> So, don't fiddle the locks without fiddling the paths.
>
> You can switch from spinning to/from blocking without touching the path, it
> figures that out.
Note that we're releasing the path shortly after. My suggestion was to
grab an *additional* read lock after atomic_inc(&eb->refs) (probably
better extent_buffer_get(eb)) and before btrfs_path_release().
An alternative would be to set path->locks[0] to NULL, which would
btrfs_release_path prevent from unlocking it, kidnapping its lock for
our purpose. But I much prefer the open coded solution.
-Jan
--
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: [PATCH 3/3] btrfs: extended inode refs
On Tue, May 08, 2012 at 03:57:39PM -0700, Mark Fasheh wrote:
> Hi Jan, comments inline as usual!
>
> > This function must not call free_extent_buffer(eb) in line 1306 after
> > applying your patch set (immediately before the break). Second, I think
> > we'd better add a blocking read lock on eb after incrementing it's
> > refcount, because we need the current content to stay as it is. Both
> > isn't part of your patches, but it might be easier if you make that
> > bugfix change as a 3/4 patch within your set and turn this one into 4/4.
> > If you don't like that, I'll send a separate patch for it. Don't miss
> > the unlock if you do it ;-)
>
> Ok, I think I was able to figure out and add the correct locking calls.
>
> Basically I believe I need to wrap access around:
>
> btrfs_tree_read_lock(eb);
> btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
>
>
>
> btrfs_tree_read_unlock_blocking(eb);
You only need a blocking lock if you're scheduling. Otherwise the
spinlock variant is fine.
> > > +
> > > + while (1) {
> > > + ret = btrfs_find_one_extref(fs_root, inum, offset, path, &iref2,
> > > + &offset);
> > > + if (ret < 0)
> > > + break;
> > > + if (ret) {
> > > + ret = found ? 0 : -ENOENT;
> > > + break;
> > > + }
> > > + ++found;
> > > +
> > > + slot = path->slots[0];
> > > + eb = path->nodes[0];
> > > + /* make sure we can use eb after releasing the path */
> > > + atomic_inc(&eb->refs);
> >
> > You need a blocking read lock here, too. Grab it before releasing the path.
If you're calling btrfs_search_slot, it will give you a blocking lock
on the leaf. If you set path->leave_spinning before the call, you'll
have a spinning lock on the leaf.
If you unlock a block that you got from a path (like eb =
path->nodes[0]), the path structure has a flag for each level that
indicates if that block was locked or not. See btrfs_release_path().
So, don't fiddle the locks without fiddling the paths.
You can switch from spinning to/from blocking without touching the path, it
figures that out.
-chris
--
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: [PATCH 3/3] btrfs: extended inode refs
Hi Jan, comments inline as usual!
On Thu, Apr 12, 2012 at 07:59:36PM +0200, Jan Schmidt wrote:
> > @@ -858,62 +859,75 @@ static int inode_ref_info(u64 inum, u64 ioff, struct
> > btrfs_root *fs_root,
> > }
> >
> > /*
> > - * this iterates to turn a btrfs_inode_ref into a full filesystem path.
> > elements
> > - * of the path are separated by '/' and the path is guaranteed to be
> > - * 0-terminated. the path is only given within the current file system.
> > - * Therefore, it never starts with a '/'. the caller is responsible to
> > provide
> > - * "size" bytes in "dest". the dest buffer will be filled backwards.
> > finally,
> > - * the start point of the resulting string is returned. this pointer is
> > within
> > - * dest, normally.
> > - * in case the path buffer would overflow, the pointer is decremented
> > further
> > - * as if output was written to the buffer, though no more output is
> > actually
> > - * generated. that way, the caller can determine how much space would be
> > - * required for the path to fit into the buffer. in that case, the returned
> > - * value will be smaller than dest. callers must check this!
> > + * Given the parent objectid and name/name_len pairs of an inode ref
> > + * (any version) this iterates to turn that information into a
> > + * full filesystem path. elements of the path are separated by '/' and
> > + * the path is guaranteed to be 0-terminated. the path is only given
> > + * within the current file system. Therefore, it never starts with a
> > + * '/'. the caller is responsible to provide "size" bytes in
> > + * "dest". the dest buffer will be filled backwards. finally, the
> > + * start point of the resulting string is returned. this pointer is
> > + * within dest, normally. in case the path buffer would overflow, the
> > + * pointer is decremented further as if output was written to the
> > + * buffer, though no more output is actually generated. that way, the
> > + * caller can determine how much space would be required for the path
> > + * to fit into the buffer. in that case, the returned value will be
> > + * smaller than dest. callers must check this!
>
> It would reduce patch sets if you can extend comments in a compatible
> way, you make reviewers happy if you don't realign text (or, later,
> function parameters) where it's not required.
Yeah I just reverted the comment change as it's no longer needed anyway.
> > */
> > static char *iref_to_path(struct btrfs_root *fs_root, struct btrfs_path
> > *path,
> > - struct btrfs_inode_ref *iref,
> > - struct extent_buffer *eb_in, u64 parent,
> > - char *dest, u32 size)
> > + int name_len, unsigned long name_off,
>
> name_len should be u32
hmm ok that's fine.
> > + struct extent_buffer *eb_in, u64 parent,
> > + char *dest, u32 size)
> > {
> > - u32 len;
> > int slot;
> > u64 next_inum;
> > int ret;
> > s64 bytes_left = size - 1;
> > struct extent_buffer *eb = eb_in;
> > struct btrfs_key found_key;
> > + struct btrfs_inode_ref *iref;
> > + struct btrfs_inode_extref *iref2;
>
> iextref
done.
> > if (bytes_left >= 0)
> > dest[bytes_left] = '\0';
> >
> > while (1) {
> > - len = btrfs_inode_ref_name_len(eb, iref);
> > - bytes_left -= len;
> > + bytes_left -= name_len;
> > if (bytes_left >= 0)
> > read_extent_buffer(eb, dest + bytes_left,
> > - (unsigned long)(iref + 1), len);
> > + name_off, name_len);
> > if (eb != eb_in)
> > free_extent_buffer(eb);
> > +
> > + /* Ok, we have enough to find any refs to the parent inode. */
> > ret = inode_ref_info(parent, 0, fs_root, path, &found_key);
> > - if (ret > 0)
> > - ret = -ENOENT;
> > - if (ret)
> > - break;
> > next_inum = found_key.offset;
> > + if (ret == 0) {
> > + slot = path->slots[0];
> > + eb = path->nodes[0];
> > + /* make sure we can use eb after releasing the path */
> > + if (eb != eb_in)
> > + atomic_inc(&eb->refs);
> > + btrfs_release_path(path);
> > + iref = btrfs_item_ptr(eb, slot, struct btrfs_inode_ref);
> > +
> > + name_len = btrfs_inode_ref_name_len(eb, iref);
> > + name_off = (unsigned long)(iref + 1);
> > + } else {
> > + ret = btrfs_find_one_extref(fs_root, parent, 0, path,
> > + &iref2, NULL);
> > + if (ret)
> > + break;
> > +
> > + next_inum = btrfs_inode_extref_pa
Re: [PATCH 3/3] btrfs: extended inode refs
On 12.04.2012 19:59, Jan Schmidt wrote: >> -static int iterate_irefs(u64 inum, struct btrfs_root *fs_root, >> -struct btrfs_path *path, >> -iterate_irefs_t *iterate, void *ctx) >> +static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root, >> + struct btrfs_path *path, >> + iterate_irefs_t *iterate, void *ctx) > > This function must not call free_extent_buffer(eb) in line 1306 after > applying your patch set (immediately before the break). Second, I think > we'd better add a blocking read lock on eb after incrementing it's > refcount, because we need the current content to stay as it is. Both > isn't part of your patches, but it might be easier if you make that > bugfix change as a 3/4 patch within your set and turn this one into 4/4. > If you don't like that, I'll send a separate patch for it. Don't miss > the unlock if you do it ;-) FYI: There are more read locks missing in the current version of backref.c. So I made a bugfix patch myself which I'll test and send tomorrow. -Jan -- 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: [PATCH 3/3] btrfs: extended inode refs
On 05.04.2012 22:09, Mark Fasheh wrote:
> The iterate_irefs in backref.c is used to build path components from inode
> refs. I had to add a 2nd iterate function callback to handle extended refs.
>
> Both iterate callbacks eventually converge upon iref_to_path() which I was
> able to keep as one function with some small code to abstract away
> differences in the two disk structures.
>
> Signed-off-by: Mark Fasheh
> ---
> fs/btrfs/backref.c | 200
> ++--
> fs/btrfs/backref.h |4 +-
> 2 files changed, 165 insertions(+), 39 deletions(-)
>
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index 0436c12..f2b8952 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -22,6 +22,7 @@
> #include "ulist.h"
> #include "transaction.h"
> #include "delayed-ref.h"
> +#include "tree-log.h"
I mentioned that in the 2/3 review mail. I'd rather get rid of this
#include here.
>
> /*
> * this structure records all encountered refs on the way up to the root
> @@ -858,62 +859,75 @@ static int inode_ref_info(u64 inum, u64 ioff, struct
> btrfs_root *fs_root,
> }
>
> /*
> - * this iterates to turn a btrfs_inode_ref into a full filesystem path.
> elements
> - * of the path are separated by '/' and the path is guaranteed to be
> - * 0-terminated. the path is only given within the current file system.
> - * Therefore, it never starts with a '/'. the caller is responsible to
> provide
> - * "size" bytes in "dest". the dest buffer will be filled backwards. finally,
> - * the start point of the resulting string is returned. this pointer is
> within
> - * dest, normally.
> - * in case the path buffer would overflow, the pointer is decremented further
> - * as if output was written to the buffer, though no more output is actually
> - * generated. that way, the caller can determine how much space would be
> - * required for the path to fit into the buffer. in that case, the returned
> - * value will be smaller than dest. callers must check this!
> + * Given the parent objectid and name/name_len pairs of an inode ref
> + * (any version) this iterates to turn that information into a
> + * full filesystem path. elements of the path are separated by '/' and
> + * the path is guaranteed to be 0-terminated. the path is only given
> + * within the current file system. Therefore, it never starts with a
> + * '/'. the caller is responsible to provide "size" bytes in
> + * "dest". the dest buffer will be filled backwards. finally, the
> + * start point of the resulting string is returned. this pointer is
> + * within dest, normally. in case the path buffer would overflow, the
> + * pointer is decremented further as if output was written to the
> + * buffer, though no more output is actually generated. that way, the
> + * caller can determine how much space would be required for the path
> + * to fit into the buffer. in that case, the returned value will be
> + * smaller than dest. callers must check this!
It would reduce patch sets if you can extend comments in a compatible
way, you make reviewers happy if you don't realign text (or, later,
function parameters) where it's not required.
> */
> static char *iref_to_path(struct btrfs_root *fs_root, struct btrfs_path
> *path,
> - struct btrfs_inode_ref *iref,
> - struct extent_buffer *eb_in, u64 parent,
> - char *dest, u32 size)
> + int name_len, unsigned long name_off,
name_len should be u32
> + struct extent_buffer *eb_in, u64 parent,
> + char *dest, u32 size)
> {
> - u32 len;
> int slot;
> u64 next_inum;
> int ret;
> s64 bytes_left = size - 1;
> struct extent_buffer *eb = eb_in;
> struct btrfs_key found_key;
> + struct btrfs_inode_ref *iref;
> + struct btrfs_inode_extref *iref2;
iextref
>
> if (bytes_left >= 0)
> dest[bytes_left] = '\0';
>
> while (1) {
> - len = btrfs_inode_ref_name_len(eb, iref);
> - bytes_left -= len;
> + bytes_left -= name_len;
> if (bytes_left >= 0)
> read_extent_buffer(eb, dest + bytes_left,
> - (unsigned long)(iref + 1), len);
> +name_off, name_len);
> if (eb != eb_in)
> free_extent_buffer(eb);
> +
> + /* Ok, we have enough to find any refs to the parent inode. */
> ret = inode_ref_info(parent, 0, fs_root, path, &found_key);
> - if (ret > 0)
> - ret = -ENOENT;
> - if (ret)
> - break;
> next_inum = found_key.offset;
> + if (ret == 0) {
> + slot = path->slots[0];
> + eb = path->nodes[0];
> +
[PATCH 3/3] btrfs: extended inode refs
The iterate_irefs in backref.c is used to build path components from inode
refs. I had to add a 2nd iterate function callback to handle extended refs.
Both iterate callbacks eventually converge upon iref_to_path() which I was
able to keep as one function with some small code to abstract away
differences in the two disk structures.
Signed-off-by: Mark Fasheh
---
fs/btrfs/backref.c | 200 ++--
fs/btrfs/backref.h |4 +-
2 files changed, 165 insertions(+), 39 deletions(-)
diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 0436c12..f2b8952 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -22,6 +22,7 @@
#include "ulist.h"
#include "transaction.h"
#include "delayed-ref.h"
+#include "tree-log.h"
/*
* this structure records all encountered refs on the way up to the root
@@ -858,62 +859,75 @@ static int inode_ref_info(u64 inum, u64 ioff, struct
btrfs_root *fs_root,
}
/*
- * this iterates to turn a btrfs_inode_ref into a full filesystem path.
elements
- * of the path are separated by '/' and the path is guaranteed to be
- * 0-terminated. the path is only given within the current file system.
- * Therefore, it never starts with a '/'. the caller is responsible to provide
- * "size" bytes in "dest". the dest buffer will be filled backwards. finally,
- * the start point of the resulting string is returned. this pointer is within
- * dest, normally.
- * in case the path buffer would overflow, the pointer is decremented further
- * as if output was written to the buffer, though no more output is actually
- * generated. that way, the caller can determine how much space would be
- * required for the path to fit into the buffer. in that case, the returned
- * value will be smaller than dest. callers must check this!
+ * Given the parent objectid and name/name_len pairs of an inode ref
+ * (any version) this iterates to turn that information into a
+ * full filesystem path. elements of the path are separated by '/' and
+ * the path is guaranteed to be 0-terminated. the path is only given
+ * within the current file system. Therefore, it never starts with a
+ * '/'. the caller is responsible to provide "size" bytes in
+ * "dest". the dest buffer will be filled backwards. finally, the
+ * start point of the resulting string is returned. this pointer is
+ * within dest, normally. in case the path buffer would overflow, the
+ * pointer is decremented further as if output was written to the
+ * buffer, though no more output is actually generated. that way, the
+ * caller can determine how much space would be required for the path
+ * to fit into the buffer. in that case, the returned value will be
+ * smaller than dest. callers must check this!
*/
static char *iref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
- struct btrfs_inode_ref *iref,
- struct extent_buffer *eb_in, u64 parent,
- char *dest, u32 size)
+ int name_len, unsigned long name_off,
+ struct extent_buffer *eb_in, u64 parent,
+ char *dest, u32 size)
{
- u32 len;
int slot;
u64 next_inum;
int ret;
s64 bytes_left = size - 1;
struct extent_buffer *eb = eb_in;
struct btrfs_key found_key;
+ struct btrfs_inode_ref *iref;
+ struct btrfs_inode_extref *iref2;
if (bytes_left >= 0)
dest[bytes_left] = '\0';
while (1) {
- len = btrfs_inode_ref_name_len(eb, iref);
- bytes_left -= len;
+ bytes_left -= name_len;
if (bytes_left >= 0)
read_extent_buffer(eb, dest + bytes_left,
- (unsigned long)(iref + 1), len);
+ name_off, name_len);
if (eb != eb_in)
free_extent_buffer(eb);
+
+ /* Ok, we have enough to find any refs to the parent inode. */
ret = inode_ref_info(parent, 0, fs_root, path, &found_key);
- if (ret > 0)
- ret = -ENOENT;
- if (ret)
- break;
next_inum = found_key.offset;
+ if (ret == 0) {
+ slot = path->slots[0];
+ eb = path->nodes[0];
+ /* make sure we can use eb after releasing the path */
+ if (eb != eb_in)
+ atomic_inc(&eb->refs);
+ btrfs_release_path(path);
+ iref = btrfs_item_ptr(eb, slot, struct btrfs_inode_ref);
+
+ name_len = btrfs_inode_ref_name_len(eb, iref);
+ name_off = (unsigned long)(iref + 1);
+ } else {
+ ret = btrfs_find_
