| Message ID | 20110802155805.GA14768@llnl.gov (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Eric Van Hensbergen |
| Headers | show |
On Tue, 2 Aug 2011 08:58:05 -0700, Jim Garlick <garlick@llnl.gov> wrote: > On Wed, Jul 20, 2011 at 05:46:05PM -0700, Eric Van Hensbergen wrote: > > On Wed, Jul 20, 2011 at 7:23 PM, Jim Garlick <garlick@llnl.gov> wrote: > > > Hi, I seem to recall a similar topic coming up on this list but can't > > > find the thread (sorry if I'm repeating earlier comments). > > > > > > For the advisory locking ops (.L), v9fs just puts the kernel l_type values, > > > e.g. F_UNLCK, F_RDLCK, and F_WRLCK on the wire. > > > > > > In my server, I have a unit test that (inadvertantly) fails if > > > l_type is defined differently on a test system compared to the system > > > where I created the test. Someone found this to be the case and > > > opened a bug: > > > > > > http://code.google.com/p/diod/issues/detail?id=69 > > > > > > We should probably be defining these values in the protocol and remapping > > > in the kernel, assuming 9P2000.L is to work in heterogenous environments. > > > > > > Thoughts? > > > > > > > Agreed. For that matter all existing flags, constants, modes, etc. > > should probably be defined in the protocol. Linux will undoubtably > > change underneath us, but then we just need to start incrementing > > protocol version to update the mappings. > > > > -eric > > Something like the following? > > I chose values for P9_LOCK_TYPE_* that will not overlap with the native > ones so servers can handle both, thus not breaking compatability with > released kernels. This seems to me to be a good general strategy for > other constants that need to be defined in the protocol. What do you think? > > Jim > > > commit 99741798ea1d1a594d9cc69174a28387ba865adf > Author: Jim Garlick <garlick.jim@gmail.com> > Date: Wed Jul 27 16:34:39 2011 -0700 > > Use protocol-defined value for lock/getlock 'type' field. signed-of-by: ? > > diff --git a/include/net/9p/9p.h b/include/net/9p/9p.h > index 071fd7a..98d9ae3 100644 > --- a/include/net/9p/9p.h > +++ b/include/net/9p/9p.h > @@ -478,6 +478,10 @@ struct p9_iattr_dotl { > #define P9_LOCK_FLAGS_BLOCK 1 > #define P9_LOCK_FLAGS_RECLAIM 2 > > +#define P9_LOCK_TYPE_RDLCK 97 > +#define P9_LOCK_TYPE_WRLCK 98 > +#define P9_LOCK_TYPE_UNLCK 99 > + Why start with 97 ?. Why not 0, 1, 2 which is the Linux values for FD_RDLCK and other ?. That will make sure we don't break the existing 9p server. > /* struct p9_flock: POSIX lock structure > * @type - type of lock > * @flags - lock flags > diff --git a/v9fs/vfs_file.c b/v9fs/vfs_file.c > index 80bddcf..4e1c620 100644 > --- a/v9fs/vfs_file.c > +++ b/v9fs/vfs_file.c > @@ -154,7 +154,17 @@ static int v9fs_file_do_lock(struct file *filp, int cmd, struct file_lock *fl) > > /* convert posix lock to p9 tlock args */ > memset(&flock, 0, sizeof(flock)); > - flock.type = fl->fl_type; > + switch (fl->fl_type) { > + case F_RDLCK: > + flock.type = P9_LOCK_TYPE_RDLCK; > + break; > + case F_WRLCK: > + flock.type = P9_LOCK_TYPE_WRLCK; > + break; > + case F_UNLCK: > + flock.type = P9_LOCK_TYPE_UNLCK; > + break; > + } > flock.start = fl->fl_start; > if (fl->fl_end == OFFSET_MAX) > flock.length = 0; > @@ -242,16 +252,25 @@ static int v9fs_file_getlock(struct file *filp, struct file_lock *fl) > res = p9_client_getlock_dotl(fid, &glock); > if (res < 0) > return res; > - if (glock.type != F_UNLCK) { > - fl->fl_type = glock.type; > + switch (glock.type) { > + case P9_LOCK_TYPE_RDLCK: > + fl->fl_type = F_RDLCK; > + break; > + case P9_LOCK_TYPE_WRLCK: > + fl->fl_type = F_WRLCK; > + break; > + case P9_LOCK_TYPE_UNLCK: > + fl->fl_type = F_UNLCK; > + break; > + } > + if (glock.type != P9_LOCK_TYPE_UNLCK) { > fl->fl_start = glock.start; > if (glock.length == 0) > fl->fl_end = OFFSET_MAX; > else > fl->fl_end = glock.start + glock.length - 1; > fl->fl_pid = glock.proc_id; > - } else > - fl->fl_type = F_UNLCK; > + } > > return res; > } > -aneesh ------------------------------------------------------------------------------ BlackBerry® DevCon Americas, Oct. 18-20, San Francisco, CA The must-attend event for mobile developers. Connect with experts. Get tools for creating Super Apps. See the latest technologies. Sessions, hands-on labs, demos & much more. Register early & save! http://p.sf.net/sfu/rim-blackberry-1
On Wed, Aug 03, 2011 at 02:37:40AM -0700, Aneesh Kumar K.V wrote: > > I chose values for P9_LOCK_TYPE_* that will not overlap with the native > > ones so servers can handle both, thus not breaking compatability with > > released kernels. This seems to me to be a good general strategy for > > other constants that need to be defined in the protocol. What do you think? > > > > +#define P9_LOCK_TYPE_RDLCK 97 > > +#define P9_LOCK_TYPE_WRLCK 98 > > +#define P9_LOCK_TYPE_UNLCK 99 > > Why start with 97 ?. Why not 0, 1, 2 which is the Linux values for > FD_RDLCK and other ?. That will make sure we don't break the existing 9p server. It does hide information from the server - e.g. someone reports a deadlock, we can't tell from a trace whether new or old locking client is used. I had assumed servers weren't really deployed yet and we could still change things, but if that's not the case, I will resubmit with 0, 1, 2. Jim ------------------------------------------------------------------------------ BlackBerry® DevCon Americas, Oct. 18-20, San Francisco, CA The must-attend event for mobile developers. Connect with experts. Get tools for creating Super Apps. See the latest technologies. Sessions, hands-on labs, demos & much more. Register early & save! http://p.sf.net/sfu/rim-blackberry-1
On Wed, Aug 03, 2011 at 09:51:29AM -0700, Jim Garlick wrote: > On Wed, Aug 03, 2011 at 02:37:40AM -0700, Aneesh Kumar K.V wrote: > > > I chose values for P9_LOCK_TYPE_* that will not overlap with the native > > > ones so servers can handle both, thus not breaking compatability with > > > released kernels. This seems to me to be a good general strategy for > > > other constants that need to be defined in the protocol. What do you think? > > > > > > +#define P9_LOCK_TYPE_RDLCK 97 > > > +#define P9_LOCK_TYPE_WRLCK 98 > > > +#define P9_LOCK_TYPE_UNLCK 99 > > > > Why start with 97 ?. Why not 0, 1, 2 which is the Linux values for > > FD_RDLCK and other ?. That will make sure we don't break the existing 9p server. > > It does hide information from the server - e.g. someone reports a deadlock, > we can't tell from a trace whether new or old locking client is used. > I had assumed servers weren't really deployed yet and we could still > change things, but if that's not the case, I will resubmit with 0, 1, 2. Oh - now I remember: If someone updates to my server with (0,1,2) patch, and doesn't update the client (likely since client is harder to update), then if they don't happen to have (0,1,2) values natively, they are newly broken. If on the other hand my server supports both F_??LCK values (the native ones) as well as new (97,98,99) ones, no new breakage is introduced. Same issue applies to the open flags patch. Maybe that is not a concern in virtfs environment? I am thinking about the guy who is passing diod through build farms on different debian architectures. Unit tests that pair a client and server on the same architecture are going to start failing if I make this change. Jim ------------------------------------------------------------------------------ BlackBerry® DevCon Americas, Oct. 18-20, San Francisco, CA The must-attend event for mobile developers. Connect with experts. Get tools for creating Super Apps. See the latest technologies. Sessions, hands-on labs, demos & much more. Register early & save! http://p.sf.net/sfu/rim-blackberry-1
On Wed, Aug 3, 2011 at 12:53 PM, Jim Garlick <garlick@llnl.gov> wrote: > On Wed, Aug 03, 2011 at 09:51:29AM -0700, Jim Garlick wrote: >> On Wed, Aug 03, 2011 at 02:37:40AM -0700, Aneesh Kumar K.V wrote: >> > > I chose values for P9_LOCK_TYPE_* that will not overlap with the native >> > > ones so servers can handle both, thus not breaking compatability with >> > > released kernels. This seems to me to be a good general strategy for >> > > other constants that need to be defined in the protocol. What do you think? >> > > >> > > +#define P9_LOCK_TYPE_RDLCK 97 >> > > +#define P9_LOCK_TYPE_WRLCK 98 >> > > +#define P9_LOCK_TYPE_UNLCK 99 >> > >> > Why start with 97 ?. Why not 0, 1, 2 which is the Linux values for >> > FD_RDLCK and other ?. That will make sure we don't break the existing 9p server. >> >> It does hide information from the server - e.g. someone reports a deadlock, >> we can't tell from a trace whether new or old locking client is used. >> I had assumed servers weren't really deployed yet and we could still >> change things, but if that's not the case, I will resubmit with 0, 1, 2. > > Oh - now I remember: > > If someone updates to my server with (0,1,2) patch, and doesn't update the > client (likely since client is harder to update), then if they don't happen > to have (0,1,2) values natively, they are newly broken. > > If on the other hand my server supports both F_??LCK values (the native ones) > as well as new (97,98,99) ones, no new breakage is introduced. > Same issue applies to the open flags patch. > > Maybe that is not a concern in virtfs environment? > > I am thinking about the guy who is passing diod through build farms on > different debian architectures. Unit tests that pair a client and server > on the same architecture are going to start failing if I make this change. > I like your idea about supporting both, but I think we need a fixed time period after which we deprecate the obsolete versions and eventually delete them. I have absolutely no idea how to set a timeline for such things, but I'm open to suggestions. I'm holding off on merging this patch until that gets resolved though. -eric ------------------------------------------------------------------------------ BlackBerry® DevCon Americas, Oct. 18-20, San Francisco, CA The must-attend event for mobile developers. Connect with experts. Get tools for creating Super Apps. See the latest technologies. Sessions, hands-on labs, demos & much more. Register early & save! http://p.sf.net/sfu/rim-blackberry-1
On Mon, Aug 08, 2011 at 02:22:56PM -0700, Eric Van Hensbergen wrote: > On Wed, Aug 3, 2011 at 12:53 PM, Jim Garlick <garlick@llnl.gov> wrote: > > On Wed, Aug 03, 2011 at 09:51:29AM -0700, Jim Garlick wrote: > >> On Wed, Aug 03, 2011 at 02:37:40AM -0700, Aneesh Kumar K.V wrote: > >> > > I chose values for P9_LOCK_TYPE_* that will not overlap with the native > >> > > ones so servers can handle both, thus not breaking compatability with > >> > > released kernels. This seems to me to be a good general strategy for > >> > > other constants that need to be defined in the protocol. What do you think? > >> > > > >> > > +#define P9_LOCK_TYPE_RDLCK 97 > >> > > +#define P9_LOCK_TYPE_WRLCK 98 > >> > > +#define P9_LOCK_TYPE_UNLCK 99 > >> > > >> > Why start with 97 ?. Why not 0, 1, 2 which is the Linux values for > >> > FD_RDLCK and other ?. That will make sure we don't break the existing 9p server. > >> > >> It does hide information from the server - e.g. someone reports a deadlock, > >> we can't tell from a trace whether new or old locking client is used. > >> I had assumed servers weren't really deployed yet and we could still > >> change things, but if that's not the case, I will resubmit with 0, 1, 2. > > > > Oh - now I remember: > > > > If someone updates to my server with (0,1,2) patch, and doesn't update the > > client (likely since client is harder to update), then if they don't happen > > to have (0,1,2) values natively, they are newly broken. > > > > If on the other hand my server supports both F_??LCK values (the native ones) > > as well as new (97,98,99) ones, no new breakage is introduced. > > Same issue applies to the open flags patch. > > > > Maybe that is not a concern in virtfs environment? > > > > I am thinking about the guy who is passing diod through build farms on > > different debian architectures. Unit tests that pair a client and server > > on the same architecture are going to start failing if I make this change. > > > > I like your idea about supporting both, but I think we need a fixed > time period after which we deprecate the obsolete versions and > eventually delete them. I have absolutely no idea how to set a > timeline for such things, but I'm open to suggestions. I'm holding > off on merging this patch until that gets resolved though. > > -eric [Sorry for the delay responding - I've been out on vacation] It does not complicate my server much at all to support both values. If the worry is not making the protocol definition look like crap, I say ignore the "pass through" mode in 2.6.38 and document the new way. Leave it to servers that want to support 2.6.38 to include support with a big comment in the code. This issue also applies to the open flags patch under consideration. IMHO we need to find a bit that is clear on all archs and set it for the offical 9P2000.L open flags definitions. I think Aneesh's concern is that virtfs/qenu is out in the wild and current versions of qemu won't work with newer kernels if my proposed scheme is accepted. I'm not sure how to make us both happy short of adding new ops with fallback in the kernel (yuck). Jim ------------------------------------------------------------------------------ FREE DOWNLOAD - uberSVN with Social Coding for Subversion. Subversion made easy with a complete admin console. Easy to use, easy to manage, easy to install, easy to extend. Get a Free download of the new open ALM Subversion platform now. http://p.sf.net/sfu/wandisco-dev2dev
On Fri, 12 Aug 2011 10:55:12 -0700, Jim Garlick <garlick@llnl.gov> wrote: > On Mon, Aug 08, 2011 at 02:22:56PM -0700, Eric Van Hensbergen wrote: > > On Wed, Aug 3, 2011 at 12:53 PM, Jim Garlick <garlick@llnl.gov> wrote: > > > On Wed, Aug 03, 2011 at 09:51:29AM -0700, Jim Garlick wrote: > > >> On Wed, Aug 03, 2011 at 02:37:40AM -0700, Aneesh Kumar K.V wrote: > > >> > > I chose values for P9_LOCK_TYPE_* that will not overlap with the native > > >> > > ones so servers can handle both, thus not breaking compatability with > > >> > > released kernels. This seems to me to be a good general strategy for > > >> > > other constants that need to be defined in the protocol. What do you think? > > >> > > > > >> > > +#define P9_LOCK_TYPE_RDLCK 97 > > >> > > +#define P9_LOCK_TYPE_WRLCK 98 > > >> > > +#define P9_LOCK_TYPE_UNLCK 99 > > >> > > > >> > Why start with 97 ?. Why not 0, 1, 2 which is the Linux values for > > >> > FD_RDLCK and other ?. That will make sure we don't break the existing 9p server. > > >> > > >> It does hide information from the server - e.g. someone reports a deadlock, > > >> we can't tell from a trace whether new or old locking client is used. > > >> I had assumed servers weren't really deployed yet and we could still > > >> change things, but if that's not the case, I will resubmit with 0, 1, 2. > > > > > > Oh - now I remember: > > > > > > If someone updates to my server with (0,1,2) patch, and doesn't update the > > > client (likely since client is harder to update), then if they don't happen > > > to have (0,1,2) values natively, they are newly broken. > > > > > > If on the other hand my server supports both F_??LCK values (the native ones) > > > as well as new (97,98,99) ones, no new breakage is introduced. > > > Same issue applies to the open flags patch. > > > > > > Maybe that is not a concern in virtfs environment? > > > > > > I am thinking about the guy who is passing diod through build farms on > > > different debian architectures. Unit tests that pair a client and server > > > on the same architecture are going to start failing if I make this change. > > > > > > > I like your idea about supporting both, but I think we need a fixed > > time period after which we deprecate the obsolete versions and > > eventually delete them. I have absolutely no idea how to set a > > timeline for such things, but I'm open to suggestions. I'm holding > > off on merging this patch until that gets resolved though. > > > > -eric > > [Sorry for the delay responding - I've been out on vacation] > > It does not complicate my server much at all to support both values. > If the worry is not making the protocol definition look like crap, > I say ignore the "pass through" mode in 2.6.38 and document the new way. > Leave it to servers that want to support 2.6.38 to include support with > a big comment in the code. > > This issue also applies to the open flags patch under consideration. > IMHO we need to find a bit that is clear on all archs and > set it for the offical 9P2000.L open flags definitions. > > I think Aneesh's concern is that virtfs/qenu is out in the wild and > current versions of qemu won't work with newer kernels if my proposed > scheme is accepted. I'm not sure how to make us both happy short of > adding new ops with fallback in the kernel (yuck). > The problem with supporting existing constant is that we won't be able to easily find the details of a deadlock is due to an older client or something else ? But it make sure that the platform where values map correctly to the new constants everything work smoothly for different combination of kernel/server. I guess that is a big advantage. Is there any other issues that i am missing when we use the existing constant values ? -aneesh ------------------------------------------------------------------------------ FREE DOWNLOAD - uberSVN with Social Coding for Subversion. Subversion made easy with a complete admin console. Easy to use, easy to manage, easy to install, easy to extend. Get a Free download of the new open ALM Subversion platform now. http://p.sf.net/sfu/wandisco-dev2dev
diff --git a/include/net/9p/9p.h b/include/net/9p/9p.h index 071fd7a..98d9ae3 100644 --- a/include/net/9p/9p.h +++ b/include/net/9p/9p.h @@ -478,6 +478,10 @@ struct p9_iattr_dotl { #define P9_LOCK_FLAGS_BLOCK 1 #define P9_LOCK_FLAGS_RECLAIM 2 +#define P9_LOCK_TYPE_RDLCK 97 +#define P9_LOCK_TYPE_WRLCK 98 +#define P9_LOCK_TYPE_UNLCK 99 + /* struct p9_flock: POSIX lock structure * @type - type of lock * @flags - lock flags diff --git a/v9fs/vfs_file.c b/v9fs/vfs_file.c index 80bddcf..4e1c620 100644 --- a/v9fs/vfs_file.c +++ b/v9fs/vfs_file.c @@ -154,7 +154,17 @@ static int v9fs_file_do_lock(struct file *filp, int cmd, struct file_lock *fl) /* convert posix lock to p9 tlock args */ memset(&flock, 0, sizeof(flock)); - flock.type = fl->fl_type; + switch (fl->fl_type) { + case F_RDLCK: + flock.type = P9_LOCK_TYPE_RDLCK; + break; + case F_WRLCK: + flock.type = P9_LOCK_TYPE_WRLCK; + break; + case F_UNLCK: + flock.type = P9_LOCK_TYPE_UNLCK; + break; + } flock.start = fl->fl_start; if (fl->fl_end == OFFSET_MAX) flock.length = 0; @@ -242,16 +252,25 @@ static int v9fs_file_getlock(struct file *filp, struct file_lock *fl) res = p9_client_getlock_dotl(fid, &glock); if (res < 0) return res; - if (glock.type != F_UNLCK) { - fl->fl_type = glock.type; + switch (glock.type) { + case P9_LOCK_TYPE_RDLCK: + fl->fl_type = F_RDLCK; + break; + case P9_LOCK_TYPE_WRLCK: + fl->fl_type = F_WRLCK; + break; + case P9_LOCK_TYPE_UNLCK: + fl->fl_type = F_UNLCK; + break; + } + if (glock.type != P9_LOCK_TYPE_UNLCK) { fl->fl_start = glock.start; if (glock.length == 0) fl->fl_end = OFFSET_MAX; else fl->fl_end = glock.start + glock.length - 1; fl->fl_pid = glock.proc_id; - } else - fl->fl_type = F_UNLCK; + } return res; }