wip-libcephfs rebased and pulled up to v.65

wip-libcephfs rebased and pulled up to v.65

[Matt W. Benjamin]

Hi Sage (et al),

We have rebased the former wip-libcephfs branch, on the model of the
rebased example branch, as planned, and also pulled it up to Ceph's
v65 tag/master, also as planned.

In addition to cross checking this, Adam has updated our Ganesha client
driver to use the ll v2 API, and this checks out.

We've pushed wip-libcephfs-rebased-v65 to our public git repository,
https://github.com/linuxbox2/linuxbox-ceph, for review.

Thanks,

Matt

-- 
Matt Benjamin
The Linux Box
206 South Fifth Ave. Suite 150
Ann Arbor, MI  48104

http://linuxbox.com

tel.  734-761-4689 
fax.  734-769-8938 
cel.  734-216-5309 

Re: wip-libcephfs rebased and pulled up to v.65

[Sage Weil]

Hi Matt,

On Tue, 2 Jul 2013, Matt W. Benjamin wrote:
> Hi Sage (et al),
> 
> We have rebased the former wip-libcephfs branch, on the model of the
> rebased example branch, as planned, and also pulled it up to Ceph's
> v65 tag/master, also as planned.
> 
> In addition to cross checking this, Adam has updated our Ganesha client
> driver to use the ll v2 API, and this checks out.
> 
> We've pushed wip-libcephfs-rebased-v65 to our public git repository,
> https://github.com/linuxbox2/linuxbox-ceph, for review.

I made a couple comments on github with small nits.  In the meantime, I'm 
going to run this through our fs test suite.

Looks good!

Ilya, do you want to rebase your changes on top of this?  It would be 
great to get both sets of changes in before the dumpling feature freeze 
(Monday!).

Thanks!
sage

HA: wip-libcephfs rebased and pulled up to v.65

[Ilya Storozhilov]

Hi Sage,

Andrey has amended our changes according to you comments except one regarding re-fetching xattrs from the MDS after setting or removal the extended attribute of the filesystem object, cause it just requires some resources we do not have at the moment (Andrey is sick till Thursday) - you can check Andrey's commits here: https://github.com/ferustigris/ceph/commits/open_by_handle_api. Other staff, including xattrs unit tests have been implemented.

Does it possible to rebase our current changes on top of this and to implement re-fetching of xattrs from MDS later, cause I suspect we are not able to complete it till Monday?

Best regards,
Ilya
________________________________________
От: Sage Weil [sage@inktank.com]
Отправлено: 3 июля 2013 г. 3:19
To: Matt W. Benjamin; Ilya Storozhilov
Cc: ceph-devel; aemerson; David Zafman
Тема: Re: wip-libcephfs rebased and pulled up to v.65

Hi Matt,

On Tue, 2 Jul 2013, Matt W. Benjamin wrote:
> Hi Sage (et al),
>
> We have rebased the former wip-libcephfs branch, on the model of the
> rebased example branch, as planned, and also pulled it up to Ceph's
> v65 tag/master, also as planned.
>
> In addition to cross checking this, Adam has updated our Ganesha client
> driver to use the ll v2 API, and this checks out.
>
> We've pushed wip-libcephfs-rebased-v65 to our public git repository,
> https://github.com/linuxbox2/linuxbox-ceph, for review.

I made a couple comments on github with small nits.  In the meantime, I'm
going to run this through our fs test suite.

Looks good!

Ilya, do you want to rebase your changes on top of this?  It would be
great to get both sets of changes in before the dumpling feature freeze
(Monday!).

Thanks!
sage
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: HA: wip-libcephfs rebased and pulled up to v.65

[Sage Weil]

On Wed, 3 Jul 2013, Ilya Storozhilov wrote:
> Hi Sage,
> 
> Andrey has amended our changes according to you comments except one 
> regarding re-fetching xattrs from the MDS after setting or removal the 
> extended attribute of the filesystem object, cause it just requires some 
> resources we do not have at the moment (Andrey is sick till Thursday) - 
> you can check Andrey's commits here: 
> https://github.com/ferustigris/ceph/commits/open_by_handle_api. Other 
> staff, including xattrs unit tests have been implemented.
> 
> Does it possible to rebase our current changes on top of this and to 
> implement re-fetching of xattrs from MDS later, cause I suspect we are 
> not able to complete it till Monday?

Sounds good; just ping me when the rebased branch is there.  I think the 
xattr pieces can wait.

Thanks!
sage

> 
> Best regards,
> Ilya
> ________________________________________
> ??: Sage Weil [sage@inktank.com]
> ??????????: 3 ???? 2013 ?. 3:19
> To: Matt W. Benjamin; Ilya Storozhilov
> Cc: ceph-devel; aemerson; David Zafman
> ????: Re: wip-libcephfs rebased and pulled up to v.65
> 
> Hi Matt,
> 
> On Tue, 2 Jul 2013, Matt W. Benjamin wrote:
> > Hi Sage (et al),
> >
> > We have rebased the former wip-libcephfs branch, on the model of the
> > rebased example branch, as planned, and also pulled it up to Ceph's
> > v65 tag/master, also as planned.
> >
> > In addition to cross checking this, Adam has updated our Ganesha client
> > driver to use the ll v2 API, and this checks out.
> >
> > We've pushed wip-libcephfs-rebased-v65 to our public git repository,
> > https://github.com/linuxbox2/linuxbox-ceph, for review.
> 
> I made a couple comments on github with small nits.  In the meantime, I'm
> going to run this through our fs test suite.
> 
> Looks good!
> 
> Ilya, do you want to rebase your changes on top of this?  It would be
> great to get both sets of changes in before the dumpling feature freeze
> (Monday!).
> 
> Thanks!
> sage
> 
> 

Re: wip-libcephfs rebased and pulled up to v.65

[Sage Weil]

Hi Matt,

This hit a problem in QA.. running 
ceph.git/qa/workunits/kernel_untar_build.sh on ceph-fuse crashes with

2013-07-03 12:51:14.176096 7fae7ee93780 10 client.4106 _lookup 10000004031.head(ref=4 cap_refs={} open={} mode=40775 size=0 mtime=2012-02-29 16:32:49.000000 caps=pAsLsXsFsx(0=pAsLsXsFsx) dirty_caps=Fx parents=0x9be18c0 0x9b4a000) generated = 1000000a29d.head(ref=3 cap_refs={} open={} mode=40775 size=0 mtime=2013-07-03 12:43:25.748972 caps=pAsLsXsFsx(0=pAsLsXsFsx) parents=0x8e909a0 0xa3b3d80)
2013-07-03 12:51:14.176126 7fae7ee93780 10 client.4106 fill_stat on 1000000a29d snap/devhead mode 040775 mtime 2013-07-03 12:43:25.748972 ctime 2013-07-03 12:43:25.748972
2013-07-03 12:51:14.176135 7fae7ee93780  3 client.4106 ll_lookup 0x9b4a000 generated -> 0 (1000000a29d)
2013-07-03 12:51:14.176148 7fae7ee93780  3 client.4106 ll_forget 10000004031 1
2013-07-03 12:51:14.191582 7fae7ee93780 -1 *** Caught signal (Segmentation fault) **
 in thread 7fae7ee93780

 ceph version 0.65-250-gfcf46bc (fcf46bc56dc37a90f69f6609e2ff3216d9195d2f)
 1: ceph-fuse() [0x7937fa]
 2: (()+0xfcb0) [0x7fae7e843cb0]
 3: (Client::_get_inodeno(Inode*)+0) [0x529a70]
 4: (Client::ll_forget(Inode*, int)+0x4a) [0x534a6a]
 5: ceph-fuse() [0x5261f5]
 6: (fuse_session_loop()+0x75) [0x7fae7ea61d65]
 7: (main()+0x848) [0x5228f8]
 8: (__libc_start_main()+0xed) [0x7fae7cd0676d]
 9: ceph-fuse() [0x523df9]
 NOTE: a copy of the executable, or `objdump -rdS <executable>` is needed to interpret this.

It wasn't immediately obvious what the problem is except that it looked 
like a (ll_)refcounting issue.  Also, the special-case of 'forget on root' 
looks suspicous; I'd like to pull in the patch from Ilya that takes a ref 
in the get_root_inode() method and make the refcounting uniform.

Thanks!
sage


On Tue, 2 Jul 2013, Matt W. Benjamin wrote:

> Hi Sage (et al),
> 
> We have rebased the former wip-libcephfs branch, on the model of the
> rebased example branch, as planned, and also pulled it up to Ceph's
> v65 tag/master, also as planned.
> 
> In addition to cross checking this, Adam has updated our Ganesha client
> driver to use the ll v2 API, and this checks out.
> 
> We've pushed wip-libcephfs-rebased-v65 to our public git repository,
> https://github.com/linuxbox2/linuxbox-ceph, for review.
> 
> Thanks,
> 
> Matt
> 
> -- 
> Matt Benjamin
> The Linux Box
> 206 South Fifth Ave. Suite 150
> Ann Arbor, MI  48104
> 
> http://linuxbox.com
> 
> tel.  734-761-4689 
> fax.  734-769-8938 
> cel.  734-216-5309 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: wip-libcephfs rebased and pulled up to v.65

[Matt W. Benjamin]

Sure.  Let us know if you'd like us to do anything in the window.

Matt

----- "Sage Weil" <sage@inktank.com> wrote:

> Hi Matt,
> 
> This hit a problem in QA.. running 
> ceph.git/qa/workunits/kernel_untar_build.sh on ceph-fuse crashes with
> 
> 2013-07-03 12:51:14.176096 7fae7ee93780 10 client.4106 _lookup
> 10000004031.head(ref=4 cap_refs={} open={} mode=40775 size=0
> mtime=2012-02-29 16:32:49.000000 caps=pAsLsXsFsx(0=pAsLsXsFsx)
> dirty_caps=Fx parents=0x9be18c0 0x9b4a000) generated =
> 1000000a29d.head(ref=3 cap_refs={} open={} mode=40775 size=0
> mtime=2013-07-03 12:43:25.748972 caps=pAsLsXsFsx(0=pAsLsXsFsx)
> parents=0x8e909a0 0xa3b3d80)
> 2013-07-03 12:51:14.176126 7fae7ee93780 10 client.4106 fill_stat on
> 1000000a29d snap/devhead mode 040775 mtime 2013-07-03 12:43:25.http://www.mindspring.com/~ffilz/748972
> ctime 2013-07-03 12:43:25.748972
> 2013-07-03 12:51:14.176135 7fae7ee93780  3 client.4106 ll_lookup
> 0x9b4a000 generated -> 0 (1000000a29d)
> 2013-07-03 12:51:14.176148 7fae7ee93780  3 client.4106 ll_forget
> 10000004031 1
> 2013-07-03 12:51:14.191582 7fae7ee93780 -1 *** Caught signal
> (Segmentation fault) **
>  in thread 7fae7ee93780
> 
>  ceph version 0.65-250-gfcf46bc
> (fcf46bc56dc37a90f69f6609e2ff3216d9195d2f)
>  1: ceph-fuse() [0x7937fa]
>  2: (()+0xfcb0) [0x7fae7e843cb0]
>  3: (Client::_get_inodeno(Inode*)+0) [0x529a70]
>  4: (Client::ll_forget(Inode*, int)+0x4a) [0x534a6a]
>  5: ceph-fuse() [0x5261f5]
>  6: (fuse_session_loop()+0x75) [0x7fae7ea61d65]
>  7: (main()+0x848) [0x5228f8]
>  8: (__libc_start_main()+0xed) [0x7fae7cd0676d]
>  9: ceph-fuse() [0x523df9]
>  NOTE: a copy of the executable, or `objdump -rdS <executable>` is
> needed to interpret this.
> 
> It wasn't immediately obvious what the problem is except that it
> looked 
> like a (ll_)refcounting issue.  Also, the special-case of 'forget on
> root' 
> looks suspicous; I'd like to pull in the patch from Ilya that takes a
> ref 
> in the get_root_inode() method and make the refcounting uniform.
> 
> Thanks!
> sage
> 
> 
> On Tue, 2 Jul 2013, Matt W. Benjamin wrote:
> 
> > Hi Sage (et al),
> > 
> > We have rebased the former wip-libcephfs branch, on the model of
> the
> > rebased example branch, as planned, and also pulled it up to Ceph's
> > v65 tag/master, also as planned.
> > 
> > In addition to cross checking this, Adam has updated our Ganesha
> client
> > driver to use the ll v2 API, and this checks out.
> > 
> > We've pushed wip-libcephfs-rebased-v65 to our public git
> repository,
> > https://github.com/linuxbox2/linuxbox-ceph, for review.
> > 
> > Thanks,
> > 
> > Matt
> > 
> > -- 
> > Matt Benjamin
> > The Linux Box
> > 206 South Fifth Ave. Suite 150
> > Ann Arbor, MI  48104
> > 
> > http://linuxbox.com
> > 
> > tel.  734-761-4689 
> > fax.  734-769-8938 
> > cel.  734-216-5309 
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> ceph-devel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> >

-- 
Matt Benjamin
The Linux Box
206 South Fifth Ave. Suite 150
Ann Arbor, MI  48104

http://linuxbox.com

tel.  734-761-4689 
fax.  734-769-8938 
cel.  734-216-5309 

Re: wip-libcephfs rebased and pulled up to v.65

[Sage Weil]

Please check out the current wip=libcephfs branch and let me know how it 
looks/works for you guys.  I cleaned up your patches a bit and fixed teh 
root cause of the xattr issue you were seeing.

Thanks!
sage


On Thu, 11 Jul 2013, Ilya Storozhilov wrote:

> Hi Adam (CC: Sage, Matt and David),
> 
> it seems to me we have choosen a bad commit description, where instead of "FIX: readlink not copy link path to user's buffer" there should be something like "FIX: ceph_ll_readlink() now fills user provided buffer with the link data instead of returning a pointer to the libcephfs internal memory location". Take a look to the sourcecode - it does actually what you are talking about (see https://github.com/ferustigris/ceph/blob/wip-libcephfs-rebased-v65/src/libcephfs.cc):
> 
> -----------------------------------------------------------------------
> extern "C" int ceph_ll_readlink(struct ceph_mount_info *cmount, Inode *in, char *buf, size_t bufsiz, int uid, int gid)
> {
>   const char *value = NULL;
>   int res = (cmount->get_client()->ll_readlink(in, &value, uid, gid));
>   if (res < 0)
>     return res;
>   if (bufsiz < (size_t)res)
>     return ENAMETOOLONG;
>   memcpy(buf, value, res);  // <-- Here we are copying a link data to the user provided buffer. This is what you want us to do.
>   return res;
> }
> -----------------------------------------------------------------------
> 
> In your branch (see https://github.com/linuxbox2/linuxbox-ceph/blob/wip-libcephfs-rebased-v65/src/libcephfs.cc) this function does not copy link data to the user-provided buffer, but passes back a pointer to the internal libcephfs structure, which is not good solution, as you mentioned below:
> 
> -----------------------------------------------------------------------
> extern "C" int ceph_ll_readlink(struct ceph_mount_info *cmount, Inode *in, char **value, int uid, int gid)
> {
>   return (cmount->get_client()->ll_readlink(in, (const char**) value, uid, gid));
> }
> -----------------------------------------------------------------------
> 
> Regards,
> Ilya
> 
> ________________________________________
> ??: Adam C. Emerson [aemerson@linuxbox.com]
> ??????????: 10 ???? 2013 ?. 20:41
> To: Ilya Storozhilov
> Cc: Sage Weil; Matt W. Benjamin; David Zafman
> ????: Re: wip-libcephfs rebased and pulled up to v.65
> 
> At Wed, 10 Jul 2013 12:17:24 +0000, Ilya Storozhilov wrote:
> [snip]
> > ?wip-libcephfs-rebased-v65? branch of the https://github.com/linuxbox2/linuxbox-ceph repository has not been branched from the ?wip-libcephfs? branch of https://github.com/ceph/ceph as it was made with our ?open_by_handle_api? branch of the https://github.com/ferustigris/ceph repo. That is why we were not able to automatically ?cherry-pick? our changes using respective git command. So we have manually applied our changes to the ?wip-libcephfs-rebased-v65? branch in https://github.com/ferustigris/ceph repo as one commit - you can check it out here: https://github.com/ferustigris/ceph/commit/c3f4940b2cfcfd3ea9a004e6f07f1aa3c0b6c419.
> [snip]
> 
> Good afternoon, sir.
> 
> I was looking at your patch and Matt and I have concerns about the
> change you made to readlink.  By passing back a pointer to a buffer
> rather than copying the link into a supplied buffer, we're opening
> ourselves up to the content changing, being deallocated, or otherwise
> having something bad happen to it.
> 
> Thanks.
> 
> 

HA: wip-libcephfs rebased and pulled up to v.65

[Ilya Storozhilov]

Hi Sage, Adam, Matt and David,

this branch is looking good at the first blush. We will check out how it works and report to you till the EOD.

Regards,
Ilya
________________________________________
От: Sage Weil [sage@inktank.com]
Отправлено: 11 июля 2013 г. 23:01
To: Ilya Storozhilov
Cc: Adam C. Emerson; Matt W. Benjamin; David Zafman; ceph-devel@vger.kernel.org
Тема: Re: wip-libcephfs rebased and pulled up to v.65

Please check out the current wip=libcephfs branch and let me know how it
looks/works for you guys.  I cleaned up your patches a bit and fixed teh
root cause of the xattr issue you were seeing.

Thanks!
sage


On Thu, 11 Jul 2013, Ilya Storozhilov wrote:

> Hi Adam (CC: Sage, Matt and David),
>
> it seems to me we have choosen a bad commit description, where instead of "FIX: readlink not copy link path to user's buffer" there should be something like "FIX: ceph_ll_readlink() now fills user provided buffer with the link data instead of returning a pointer to the libcephfs internal memory location". Take a look to the sourcecode - it does actually what you are talking about (see https://github.com/ferustigris/ceph/blob/wip-libcephfs-rebased-v65/src/libcephfs.cc):
>
> -----------------------------------------------------------------------
> extern "C" int ceph_ll_readlink(struct ceph_mount_info *cmount, Inode *in, char *buf, size_t bufsiz, int uid, int gid)
> {
>   const char *value = NULL;
>   int res = (cmount->get_client()->ll_readlink(in, &value, uid, gid));
>   if (res < 0)
>     return res;
>   if (bufsiz < (size_t)res)
>     return ENAMETOOLONG;
>   memcpy(buf, value, res);  // <-- Here we are copying a link data to the user provided buffer. This is what you want us to do.
>   return res;
> }
> -----------------------------------------------------------------------
>
> In your branch (see https://github.com/linuxbox2/linuxbox-ceph/blob/wip-libcephfs-rebased-v65/src/libcephfs.cc) this function does not copy link data to the user-provided buffer, but passes back a pointer to the internal libcephfs structure, which is not good solution, as you mentioned below:
>
> -----------------------------------------------------------------------
> extern "C" int ceph_ll_readlink(struct ceph_mount_info *cmount, Inode *in, char **value, int uid, int gid)
> {
>   return (cmount->get_client()->ll_readlink(in, (const char**) value, uid, gid));
> }
> -----------------------------------------------------------------------
>
> Regards,
> Ilya
>
> ________________________________________
> ??: Adam C. Emerson [aemerson@linuxbox.com]
> ??????????: 10 ???? 2013 ?. 20:41
> To: Ilya Storozhilov
> Cc: Sage Weil; Matt W. Benjamin; David Zafman
> ????: Re: wip-libcephfs rebased and pulled up to v.65
>
> At Wed, 10 Jul 2013 12:17:24 +0000, Ilya Storozhilov wrote:
> [snip]
> > ?wip-libcephfs-rebased-v65? branch of the https://github.com/linuxbox2/linuxbox-ceph repository has not been branched from the ?wip-libcephfs? branch of https://github.com/ceph/ceph as it was made with our ?open_by_handle_api? branch of the https://github.com/ferustigris/ceph repo. That is why we were not able to automatically ?cherry-pick? our changes using respective git command. So we have manually applied our changes to the ?wip-libcephfs-rebased-v65? branch in https://github.com/ferustigris/ceph repo as one commit - you can check it out here: https://github.com/ferustigris/ceph/commit/c3f4940b2cfcfd3ea9a004e6f07f1aa3c0b6c419.
> [snip]
>
> Good afternoon, sir.
>
> I was looking at your patch and Matt and I have concerns about the
> change you made to readlink.  By passing back a pointer to a buffer
> rather than copying the link into a supplied buffer, we're opening
> ourselves up to the content changing, being deallocated, or otherwise
> having something bad happen to it.
>
> Thanks.
>
>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html