[PATCH] Btrfs: fix regression in re-setting a large xattr
Recently I changed the xattr stuff to unconditionally set the xattr first in
case the xattr didn't exist yet. This has introduced a regression when setting
an xattr that already exists with a large value. If we find the key we are
looking for split_leaf will assume that we're extending that item. The problem
is the size we pass down to btrfs_search_slot includes the size of the item
already, so if we have the largest xattr we can possibly have plus the size of
the xattr item plus the xattr item that btrfs_search_slot we'd overflow the
leaf. Thankfully this is not what we're doing, but split_leaf doesn't know this
so it just returns EOVERFLOW. So in the xattr code we need to check and see if
we got back EOVERFLOW and treat it like EEXIST since that's really what
happened. Thanks,
Signed-off-by: Josef Bacik
---
fs/btrfs/xattr.c | 13 -
1 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 69565e5..5bd7877 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -127,7 +127,18 @@ static int do_setxattr(struct btrfs_trans_handle *trans,
again:
ret = btrfs_insert_xattr_item(trans, root, path, btrfs_ino(inode),
name, name_len, value, size);
- if (ret == -EEXIST) {
+ /*
+* If we're setting an xattr to a new value but the new value is say
+* exactly BTRFS_MAX_XATTR_SIZE, we could end up with EOVERFLOW getting
+* back from split_leaf. This is because it thinks we'll be extending
+* the existing item size, but we're asking for enough space to add the
+* item itself. So if we get EOVERFLOW just set ret to EEXIST and let
+* the rest of the function figure it out.
+*/
+ if (ret == -EOVERFLOW)
+ ret = -EEXIST;
+
+ if (ret == -EEXIST || ret == -EOVERFLOW) {
if (flags & XATTR_CREATE)
goto out;
/*
--
1.7.5.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
Re: [PATCH] Btrfs: fix regression in re-setting a large xattr
(2011/10/14 2:11), Josef Bacik wrote:
> Recently I changed the xattr stuff to unconditionally set the xattr first in
> case the xattr didn't exist yet. This has introduced a regression when
> setting
> an xattr that already exists with a large value. If we find the key we are
> looking for split_leaf will assume that we're extending that item. The
> problem
> is the size we pass down to btrfs_search_slot includes the size of the item
> already, so if we have the largest xattr we can possibly have plus the size of
> the xattr item plus the xattr item that btrfs_search_slot we'd overflow the
> leaf. Thankfully this is not what we're doing, but split_leaf doesn't know
> this
> so it just returns EOVERFLOW. So in the xattr code we need to check and see
> if
> we got back EOVERFLOW and treat it like EEXIST since that's really what
> happened. Thanks,
>
> Signed-off-by: Josef Bacik
> ---
> fs/btrfs/xattr.c | 13 -
> 1 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index 69565e5..5bd7877 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -127,7 +127,18 @@ static int do_setxattr(struct btrfs_trans_handle *trans,
> again:
> ret = btrfs_insert_xattr_item(trans, root, path, btrfs_ino(inode),
> name, name_len, value, size);
> - if (ret == -EEXIST) {
> + /*
> + * If we're setting an xattr to a new value but the new value is say
> + * exactly BTRFS_MAX_XATTR_SIZE, we could end up with EOVERFLOW getting
> + * back from split_leaf. This is because it thinks we'll be extending
> + * the existing item size, but we're asking for enough space to add the
> + * item itself. So if we get EOVERFLOW just set ret to EEXIST and let
> + * the rest of the function figure it out.
> + */
> + if (ret == -EOVERFLOW)
> + ret = -EEXIST;
> +
> + if (ret == -EEXIST || ret == -EOVERFLOW) {
Why tested again EOVERFLOW?
Thanks,
Tsutomu
> if (flags & XATTR_CREATE)
> goto out;
> /*
--
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] Btrfs: fix regression in re-setting a large xattr
On Fri, Oct 14, 2011 at 09:04:28AM +0900, Tsutomu Itoh wrote:
> (2011/10/14 2:11), Josef Bacik wrote:
> > Recently I changed the xattr stuff to unconditionally set the xattr first in
> > case the xattr didn't exist yet. This has introduced a regression when
> > setting
> > an xattr that already exists with a large value. If we find the key we are
> > looking for split_leaf will assume that we're extending that item. The
> > problem
> > is the size we pass down to btrfs_search_slot includes the size of the item
> > already, so if we have the largest xattr we can possibly have plus the size
> > of
> > the xattr item plus the xattr item that btrfs_search_slot we'd overflow the
> > leaf. Thankfully this is not what we're doing, but split_leaf doesn't know
> > this
> > so it just returns EOVERFLOW. So in the xattr code we need to check and
> > see if
> > we got back EOVERFLOW and treat it like EEXIST since that's really what
> > happened. Thanks,
> >
> > Signed-off-by: Josef Bacik
> > ---
> > fs/btrfs/xattr.c | 13 -
> > 1 files changed, 12 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> > index 69565e5..5bd7877 100644
> > --- a/fs/btrfs/xattr.c
> > +++ b/fs/btrfs/xattr.c
> > @@ -127,7 +127,18 @@ static int do_setxattr(struct btrfs_trans_handle
> > *trans,
> > again:
> > ret = btrfs_insert_xattr_item(trans, root, path, btrfs_ino(inode),
> > name, name_len, value, size);
> > - if (ret == -EEXIST) {
> > + /*
> > +* If we're setting an xattr to a new value but the new value is say
> > +* exactly BTRFS_MAX_XATTR_SIZE, we could end up with EOVERFLOW getting
> > +* back from split_leaf. This is because it thinks we'll be extending
> > +* the existing item size, but we're asking for enough space to add the
> > +* item itself. So if we get EOVERFLOW just set ret to EEXIST and let
> > +* the rest of the function figure it out.
> > +*/
> > + if (ret == -EOVERFLOW)
> > + ret = -EEXIST;
> > +
> > + if (ret == -EEXIST || ret == -EOVERFLOW) {
>
> Why tested again EOVERFLOW?
>
Oops thats my fault, I had thought to check for eoverflow but then thought
better to just set it to eexist and didn't fix the first thought. I'll send out
a fix. Thanks,
Josef
--
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: fix regression in re-setting a large xattr V2
Recently I changed the xattr stuff to unconditionally set the xattr first in
case the xattr didn't exist yet. This has introduced a regression when setting
an xattr that already exists with a large value. If we find the key we are
looking for split_leaf will assume that we're extending that item. The problem
is the size we pass down to btrfs_search_slot includes the size of the item
already, so if we have the largest xattr we can possibly have plus the size of
the xattr item plus the xattr item that btrfs_search_slot we'd overflow the
leaf. Thankfully this is not what we're doing, but split_leaf doesn't know this
so it just returns EOVERFLOW. So in the xattr code we need to check and see if
we got back EOVERFLOW and treat it like EEXIST since that's really what
happened. Thanks,
Signed-off-by: Josef Bacik
---
V1->V2: fix an extra overflow check that i forgot to remove
fs/btrfs/xattr.c | 11 +++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 69565e5..a76e41c 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -127,6 +127,17 @@ static int do_setxattr(struct btrfs_trans_handle *trans,
again:
ret = btrfs_insert_xattr_item(trans, root, path, btrfs_ino(inode),
name, name_len, value, size);
+ /*
+* If we're setting an xattr to a new value but the new value is say
+* exactly BTRFS_MAX_XATTR_SIZE, we could end up with EOVERFLOW getting
+* back from split_leaf. This is because it thinks we'll be extending
+* the existing item size, but we're asking for enough space to add the
+* item itself. So if we get EOVERFLOW just set ret to EEXIST and let
+* the rest of the function figure it out.
+*/
+ if (ret == -EOVERFLOW)
+ ret = -EEXIST;
+
if (ret == -EEXIST) {
if (flags & XATTR_CREATE)
goto out;
--
1.7.5.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
