Re: Junior Kernel Hacker task: improve vnode-v_tag
Chris Costello wrote:
On Saturday, September 08, 2001, Maxim Sobolev wrote:
I don't like idea to hardcode the same string (procfs), with the
same meaning in several places across kernel. As for your proposition
to use f_fstypename to set v_tag, it is even more bogus because
value of the f_fstypename is supplied from the user level, so
potentially it could be anything and we can't make any reasonable
assumptions about mapping between its value and type of the filesystem
in question.
How do you figure? The contents if `f_fstypename' must match
a configured file system exactly, so it could _not_ be anything.
To quote sys/kern/vfs_syscalls.c:mount():
Oh, yes, you are correct obviously (don't know what I was thinking about). In this
case, it looks like v_tag is redundant, because f_fstypename could be used instead
in a few places where v_tag is abused (the same applies to the statfs.f_type
because essentually it is the same thing as v_tag). Poul, what do you think about
it? In the meantime, I found another place in the kernel where VT_* macros are
[ab]used - it is Linuxlator, attached please find patches to fix it - please
review.
-Maxim
Index: linux_stats.c
===
RCS file: /home/ncvs/src/sys/compat/linux/linux_stats.c,v
retrieving revision 1.37
diff -d -u -r1.37 linux_stats.c
--- linux_stats.c 2001/09/12 08:36:57 1.37
+++ linux_stats.c 2001/09/18 11:52:02
@@ -187,10 +187,6 @@
l_int f_spare[6];
};
-#ifndef VT_NWFS
-#defineVT_NWFS VT_TFS /* XXX - bug compat. with sys/fs/nwfs/nwfs_node.h */
-#endif
-
#defineLINUX_CODA_SUPER_MAGIC 0x73757245L
#defineLINUX_EXT2_SUPER_MAGIC 0xEF53L
#defineLINUX_HPFS_SUPER_MAGIC 0xf995e849L
@@ -202,34 +198,30 @@
#defineLINUX_PROC_SUPER_MAGIC 0x9fa0L
#defineLINUX_UFS_SUPER_MAGIC 0x00011954L /* XXX - UFS_MAGIC in Linux */
-/*
- * ext2fs uses the VT_UFS tag. A mounted ext2 filesystem will therefore
- * be seen as an ufs filesystem.
- */
static long
-bsd_to_linux_ftype(int tag)
+bsd_to_linux_ftype(const char *fstypename)
{
- switch (tag) {
- case VT_CODA:
+ if (strcmp(fstypename, coda) == 0)
return (LINUX_CODA_SUPER_MAGIC);
- case VT_HPFS:
+ else if (strcmp(fstypename, hpfs) == 0)
return (LINUX_HPFS_SUPER_MAGIC);
- case VT_ISOFS:
+ else if (strcmp(fstypename, cd9660) == 0)
return (LINUX_ISOFS_SUPER_MAGIC);
- case VT_MSDOSFS:
+ else if (strcmp(fstypename, msdosfs) == 0)
return (LINUX_MSDOS_SUPER_MAGIC);
- case VT_NFS:
+ else if (strcmp(fstypename, nfs) == 0)
return (LINUX_NFS_SUPER_MAGIC);
- case VT_NTFS:
+ else if (strcmp(fstypename, ntfs) == 0)
return (LINUX_NTFS_SUPER_MAGIC);
- case VT_NWFS:
+ else if (strcmp(fstypename, nwfs) == 0)
return (LINUX_NCP_SUPER_MAGIC);
- case VT_PROCFS:
+ else if (strcmp(fstypename, procfs) == 0)
return (LINUX_PROC_SUPER_MAGIC);
- case VT_UFS:
+ else if (strcmp(fstypename, ufs) == 0)
return (LINUX_UFS_SUPER_MAGIC);
- }
+ else if (strcmp(fstypename, ext2fs) == 0)
+ return (LINUX_EXT2_SUPER_MAGIC);
return (0L);
}
@@ -265,7 +257,7 @@
if (error)
return error;
bsd_statfs-f_flags = mp-mnt_flag MNT_VISFLAGMASK;
- linux_statfs.f_type = bsd_to_linux_ftype(bsd_statfs-f_type);
+ linux_statfs.f_type = bsd_to_linux_ftype(bsd_statfs-f_fstypename);
linux_statfs.f_bsize = bsd_statfs-f_bsize;
linux_statfs.f_blocks = bsd_statfs-f_blocks;
linux_statfs.f_bfree = bsd_statfs-f_bfree;
@@ -301,7 +293,7 @@
if (error)
return error;
bsd_statfs-f_flags = mp-mnt_flag MNT_VISFLAGMASK;
- linux_statfs.f_type = bsd_to_linux_ftype(bsd_statfs-f_type);
+ linux_statfs.f_type = bsd_to_linux_ftype(bsd_statfs-f_fstypename);
linux_statfs.f_bsize = bsd_statfs-f_bsize;
linux_statfs.f_blocks = bsd_statfs-f_blocks;
linux_statfs.f_bfree = bsd_statfs-f_bfree;
Re: Junior Kernel Hacker task: improve vnode-v_tag
On Tuesday, September 18, 2001, Maxim Sobolev wrote: Oh, yes, you are correct obviously (don't know what I was thinking about). In this case, it looks like v_tag is redundant, because f_fstypename could be used instead in a few places where v_tag is abused (the same applies to the statfs.f_type because essentually it is the same thing as v_tag). Poul, what do you think about it? In the meantime, I found another place in the kernel where VT_* macros are [ab]used - it is Linuxlator, attached please find patches to fix it - please review. Actually, I've got work that's a lot like the patch you attached to this message; when I can merge some of the latest changes, I'll have a diff at least to KSE_PRE_MILESTONE_2. Give me more time and I'll have a diff to HEAD. -- +---++ | Chris Costello| Performance is easier to add than clarity. | | [EMAIL PROTECTED] || +---++ To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
In message [EMAIL PROTECTED], Maxim Sobolev writes: How do you figure? The contents if `f_fstypename' must match a configured file system exactly, so it could _not_ be anything. To quote sys/kern/vfs_syscalls.c:mount(): Oh, yes, you are correct obviously (don't know what I was thinking about). In this case, it looks like v_tag is redundant, because f_fstypename could be used instead in a few places where v_tag is abused (the same applies to the statfs.f_type because essentually it is the same thing as v_tag). Poul, what do you think about it? In the meantime, I found another place in the kernel where VT_* macros are [ab]used - it is Linuxlator, attached please find patches to fix it - please review. Sounds like a plan, and a quick eyeball of the patch shows no trouble. Poul-Henning -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
On Tue, Sep 18, 2001 at 03:09:33PM +0300, Maxim Sobolev wrote: The patch looks ok. There's a slight functional change that an ext2fs filesystem is now correctly returned as such. I don't expect Linux binaries to break, but it may be remotely possible that certain tools, now that they detect ext2fs filesystems, will use more specific ext2fs queries for whatever reason they need. I don't think it's something to worry about... - case VT_UFS: + else if (strcmp(fstypename, ufs) == 0) return (LINUX_UFS_SUPER_MAGIC); - } + else if (strcmp(fstypename, ext2fs) == 0) + return (LINUX_EXT2_SUPER_MAGIC); -- Marcel Moolenaar USPA: A-39004 [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
On Sat, 8 Sep 2001, Chris Costello wrote: On Saturday, September 08, 2001, Poul-Henning Kamp wrote: No actually not, I want something short and predictable like VT_CODA. How about my second suggestion: making v_tag point to mp-mnt_stat.f_fstypename, or a copy thereof? Good, but I as far as I understand this, the only legitimate point of v_tag is to tell applications like fstat(1) the type of the vnode. fstat can find chase the kernel pointers in vp-v_mount-mnt_stat-f_fstypename almost as (un)easily as it could chase vp-v_tag if v_tag is a pointer. Copying the string would give ugly bloat, but would not be all that much worse than the bloat for a pointer on alphas (the contents of the string VT_CODA takes the same space as a pointer on alphas). Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
On Saturday, September 08, 2001, Maxim Sobolev wrote:
No, it should be pre-defined, because otherwise we will be
unable to use strcmp() in a few places when v_tag is abused.
So in these cases (which ideally would be eliminated rather
than considered for support), why can't you do:
if (strcmp(vp-v_tag, procfs) == 0) {
rather than
if (strcmp(vp-v_tag, VT_PROCFS) == 0) {
in the case of procfs? As for a solution to the problem, I'm
not entirely sure, but maybe this is a case for either a new
vnode flag, `VUNSAFE', for files which should be closed across
exec() calls (this is all setugidsafety() and its hackish
is_unsafe() companion are used for as far as I can tell).
--
+---++
| Chris Costello| Let the machine do the dirty work. |
| [EMAIL PROTECTED] |- Elements of Programming Style |
+---++
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
On Saturday, September 08, 2001, Maxim Sobolev wrote:
I don't like idea to hardcode the same string (procfs), with the
same meaning in several places across kernel. As for your proposition
to use f_fstypename to set v_tag, it is even more bogus because
value of the f_fstypename is supplied from the user level, so
potentially it could be anything and we can't make any reasonable
assumptions about mapping between its value and type of the filesystem
in question.
How do you figure? The contents if `f_fstypename' must match
a configured file system exactly, so it could _not_ be anything.
To quote sys/kern/vfs_syscalls.c:mount():
if ((error = copyinstr(SCARG(uap, type), fstypename, MFSNAMELEN, NULL))
vput(vp);
return (error);
}
for (vfsp = vfsconf; vfsp; vfsp = vfsp-vfc_next)
if (!strcmp(vfsp-vfc_name, fstypename))
break;
if (vfsp == NULL) {
/* ... try and load a module ... */
/* ... fail if no module can be found, or no
* matching VFS is loaded */
}
--
+---+---+
| Chris Costello| You depend too much on computers for information. |
| [EMAIL PROTECTED] | |
+---+---+
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
On Tuesday, September 04, 2001, Maxim Sobolev wrote: Content-Description: ASCII C program text Index: coda/coda.h === RCS file: /home/ncvs/src/sys/coda/coda.h,v retrieving revision 1.9 diff -d -u -r1.9 coda.h --- coda/coda.h 1999/12/29 04:54:30 1.9 +++ coda/coda.h 2001/09/04 18:46:42 @@ -41,7 +41,7 @@ #ifndef _CODA_HEADER_ #define _CODA_HEADER_ - +#define VT_CODA VT_CODA ... I don't think that the point of this is to use a string like that, but rather a descriptive string, i.e. #define VT_FDESCFS file-descriptor file system #define VT_NFS network file system But is it necessary that you really use those defines? The idea is not to use them globally. Perhaps getnewvnode() should get the string from `mp-mnt_stat.f_mntfromname', instead... -- +---+---+ | Chris Costello| As far as we know, our computer has never | | [EMAIL PROTECTED] | had an undetected error.- Weisert | +---+---+ To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
On Friday, September 07, 2001, Chris Costello wrote: But is it necessary that you really use those defines? The idea is not to use them globally. Perhaps getnewvnode() should get the string from `mp-mnt_stat.f_mntfromname', instead... ^ Er, fstypename... -- +---++ | Chris Costello| Unprecedented performance: | | [EMAIL PROTECTED] | Nothing ever ran this slow before. | +---++ To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
In message [EMAIL PROTECTED], Chris Costello writes: On Tuesday, September 04, 2001, Maxim Sobolev wrote: Content-Description: ASCII C program text Index: coda/coda.h === RCS file: /home/ncvs/src/sys/coda/coda.h,v retrieving revision 1.9 diff -d -u -r1.9 coda.h --- coda/coda.h 1999/12/29 04:54:30 1.9 +++ coda/coda.h 2001/09/04 18:46:42 @@ -41,7 +41,7 @@ #ifndef _CODA_HEADER_ #define _CODA_HEADER_ - +#define VT_CODA VT_CODA ... I don't think that the point of this is to use a string like that, but rather a descriptive string, i.e. No actually not, I want something short and predictable like VT_CODA. -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
On Saturday, September 08, 2001, Poul-Henning Kamp wrote: No actually not, I want something short and predictable like VT_CODA. How about my second suggestion: making v_tag point to mp-mnt_stat.f_fstypename, or a copy thereof? -- +---+--+ | Chris Costello| Why do we want intelligent terminals | | [EMAIL PROTECTED] | when there are so many stupid users? | +---+--+ To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
While on the subject of VFS locking... Accessing devfs through a nullfs redirection causes a panic() due to locking issues. I haven't had time to look at this in detail yet, if somebody wants to jump up and fix the problem, feel free... -Jon To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Junior Kernel Hacker task: improve vnode-v_tag
Assignment: The v_tag element in struct vnode is a debugging aid, but unfortunately it is implemented in a way which means that adding a filesystem means modifying the definition in sys/vnode.h. Convert the v_tag to an const char * and have the filesystems put their name in there instead. The v_tag has been abused a few places, easily recognizable by the fact that the kernel should never inspect the value of v_tag. These places should be easily changeable to use the new representation. Please mark them with a big fat /*XXX: ABUSE OF v_tag */ comment. -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
On 04 Sep 2001 at 10:36 (+0200), Poul-Henning Kamp wrote: | | Assignment: | | The v_tag element in struct vnode is a debugging aid, but unfortunately | it is implemented in a way which means that adding a filesystem means | modifying the definition in sys/vnode.h. | | Convert the v_tag to an const char * and have the filesystems put | their name in there instead. | | The v_tag has been abused a few places, easily recognizable by the fact | that the kernel should never inspect the value of v_tag. | These places should be easily changeable to use the new representation. | Please mark them with a big fat /*XXX: ABUSE OF v_tag */ comment. #include newbie_kernel_hacker_warning.h I've done a /cursory/ look over how this v_tag is used. I'm not sure this is a simple/clean as you propose, since this is used in the IS_LOCKING_VFS macro, as well as in union_subr.c... These uses /seem/ fairly significant due to the fact that the v_tag is being used to determine capabilities of the vnode in question. How should this be worked around? It would be fairly trivial to change v_tag to v_feature (or similar) then add the v_tag as you propose. I suggest this since those _tests_ for features should IMO not be done with a char[] operation, since that would (in my ignorant estimation) have a very negative effect on fs performance. Of course, my suggested v_feature hack would again require changing vnode.h for new features, but this is (assumedly) less frequent than modifying for file systems. I'll work on this this weekend, and (hopefully) follow thru with a patch. cheers. Brent -- Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing. -- Duane Allman To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
In message [EMAIL PROTECTED], Brent Verner writes: On 04 Sep 2001 at 10:36 (+0200), Poul-Henning Kamp wrote: | | Assignment: | | The v_tag element in struct vnode is a debugging aid, but unfortunately | it is implemented in a way which means that adding a filesystem means | modifying the definition in sys/vnode.h. | | Convert the v_tag to an const char * and have the filesystems put | their name in there instead. | | The v_tag has been abused a few places, easily recognizable by the fact | that the kernel should never inspect the value of v_tag. | These places should be easily changeable to use the new representation. | Please mark them with a big fat /*XXX: ABUSE OF v_tag */ comment. #include newbie_kernel_hacker_warning.h I've done a /cursory/ look over how this v_tag is used. I'm not sure this is a simple/clean as you propose, since this is used in the IS_LOCKING_VFS macro, as well as in union_subr.c... Well, that is just too bad, because IS_LOCKING_VFS is wrong then. The places which inspect v_tag will have to be changed to use strcmp() then... -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
On Tue, Sep 04, 2001 at 02:27:00PM +0200, Poul-Henning Kamp wrote: I've done a /cursory/ look over how this v_tag is used. I'm not sure this is a simple/clean as you propose, since this is used in the IS_LOCKING_VFS macro, as well as in union_subr.c... Well, that is just too bad, because IS_LOCKING_VFS is wrong then. That's what the line of comment above it said ;) /* * [dfr] Kludge until I get around to fixing all the vfs locking. */ Mark -- Mark Santcroos RIPE Network Coordination Centre http://www.ripe.net/home/mark/ New Projects Group/TTM To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
In message [EMAIL PROTECTED], Maxim Sobolev writes:
In message [EMAIL PROTECTED], Brent Verner writes:
I've done a /cursory/ look over how this v_tag is used. I'm not sure
this is a simple/clean as you propose, since this is used in the
IS_LOCKING_VFS macro, as well as in union_subr.c...
Well, that is just too bad, because IS_LOCKING_VFS is wrong then.
The places which inspect v_tag will have to be changed to use
strcmp() then...
I think that we can add a new vnode flag, say VCANLOCK, so that each
particular VFS can set it if it supports locking, which should allow
to remove pre-defined VFS list from the IS_LOCKING_VFS macro. I can
produce a patch if it sounds reasonably.
Yeah, I think that makes a lot of sense.
See attached. Please let me know if it is OK for you.
-Maxim
Index: isofs/cd9660/cd9660_vfsops.c
===
RCS file: /home/ncvs/src/sys/isofs/cd9660/cd9660_vfsops.c,v
retrieving revision 1.91
diff -d -u -r1.91 cd9660_vfsops.c
--- isofs/cd9660/cd9660_vfsops.c2001/05/16 18:04:30 1.91
+++ isofs/cd9660/cd9660_vfsops.c2001/09/04 15:20:46
@@ -697,6 +697,7 @@
}
MALLOC(ip, struct iso_node *, sizeof(struct iso_node), M_ISOFSNODE,
M_WAITOK | M_ZERO);
+ vp-v_flag |= VLOCKABLE;
lockinit(vp-v_lock, PINOD, isonode, 0, 0);
/*
* ISOFS uses stdlock and can share lock structure
Index: ufs/ffs/ffs_vfsops.c
===
RCS file: /home/ncvs/src/sys/ufs/ffs/ffs_vfsops.c,v
retrieving revision 1.157
diff -d -u -r1.157 ffs_vfsops.c
--- ufs/ffs/ffs_vfsops.c2001/06/28 22:21:27 1.157
+++ ufs/ffs/ffs_vfsops.c2001/09/04 15:21:25
@@ -1172,6 +1172,7 @@
return (error);
}
bzero((caddr_t)ip, sizeof(struct inode));
+ vp-v_flag |= VLOCKABLE;
/*
* FFS supports lock sharing in the stack of vnodes
*/
Index: ufs/ifs/ifs_vfsops.c
===
RCS file: /home/ncvs/src/sys/ufs/ifs/ifs_vfsops.c,v
retrieving revision 1.6
diff -d -u -r1.6 ifs_vfsops.c
--- ufs/ifs/ifs_vfsops.c2001/04/25 07:07:51 1.6
+++ ufs/ifs/ifs_vfsops.c2001/09/04 15:21:25
@@ -217,6 +217,7 @@
return (error);
}
bzero((caddr_t)ip, sizeof(struct inode));
+ vp-v_flag |= VLOCKABLE;
/*
* IFS supports lock sharing in the stack of vnodes
*/
Index: nfs/nfs_node.c
===
RCS file: /home/ncvs/src/sys/nfs/nfs_node.c,v
retrieving revision 1.49
diff -d -u -r1.49 nfs_node.c
--- nfs/nfs_node.c 2001/05/01 08:13:14 1.49
+++ nfs/nfs_node.c 2001/09/04 15:21:25
@@ -232,6 +232,7 @@
}
vp = nvp;
bzero((caddr_t)np, sizeof *np);
+ vp-v_flag |= VLOCKABLE;
vp-v_data = np;
np-n_vnode = vp;
/*
Index: sys/vnode.h
===
RCS file: /home/ncvs/src/sys/sys/vnode.h,v
retrieving revision 1.154
diff -d -u -r1.154 vnode.h
--- sys/vnode.h 2001/08/27 06:09:55 1.154
+++ sys/vnode.h 2001/09/04 15:21:25
@@ -175,6 +175,7 @@
/* open for business 0x10 */
#defineVONWORKLST 0x20 /* On syncer work-list */
#defineVMOUNT 0x40 /* Mount in progress */
+#define VLOCKABLE 0x60 /* vnode supports locking */
/*
* Vnode attributes. A field value of VNOVAL represents a field whose value
@@ -433,12 +434,7 @@
/*
* [dfr] Kludge until I get around to fixing all the vfs locking.
*/
-#define IS_LOCKING_VFS(vp) ((vp)-v_tag == VT_UFS \
-|| (vp)-v_tag == VT_NFS \
-|| (vp)-v_tag == VT_LFS \
-|| (vp)-v_tag == VT_ISOFS \
-|| (vp)-v_tag == VT_MSDOSFS \
-|| (vp)-v_tag == VT_DEVFS)
+#define IS_LOCKING_VFS(vp) ((vp)-v_flag VLOCKABLE)
#define ASSERT_VOP_LOCKED(vp, str) \
do { \
Index: fs/devfs/devfs_vnops.c
===
RCS file: /home/ncvs/src/sys/fs/devfs/devfs_vnops.c,v
retrieving revision 1.27
diff -d -u -r1.27 devfs_vnops.c
--- fs/devfs/devfs_vnops.c 2001/08/14 06:42:32 1.27
+++ fs/devfs/devfs_vnops.c 2001/09/04 15:21:25
@@ -151,6 +151,7 @@
} else {
vp-v_type = VBAD;
}
+ vp-v_flag |= VLOCKABLE;
vp-v_data = de;
de-de_vnode = vp;
vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p);
Index: fs/msdosfs/msdosfs_denode.c
===
RCS
Re: Junior Kernel Hacker task: improve vnode-v_tag
Hi Maxim, Perhaps you meant: diff -d -u -r1.154 vnode.h --- sys/vnode.h 2001/08/27 06:09:55 1.154 +++ sys/vnode.h 2001/09/04 15:21:25 @@ -175,6 +175,7 @@ /* open for business 0x10 */ #defineVONWORKLST 0x20 /* On syncer work-list */ #defineVMOUNT 0x40 /* Mount in progress */ +#define VLOCKABLE 0x60 /* vnode supports locking */ ...should be +#define VLOCKABLE 0x80 /* vnode supports locking */ ? Regards, Konstantin. Maxim Sobolev wrote: In message [EMAIL PROTECTED], Maxim Sobolev writes: In message [EMAIL PROTECTED], Brent Verner writes: I've done a /cursory/ look over how this v_tag is used. I'm not sure this is a simple/clean as you propose, since this is used in the IS_LOCKING_VFS macro, as well as in union_subr.c... Well, that is just too bad, because IS_LOCKING_VFS is wrong then. The places which inspect v_tag will have to be changed to use strcmp() then... I think that we can add a new vnode flag, say VCANLOCK, so that each particular VFS can set it if it supports locking, which should allow to remove pre-defined VFS list from the IS_LOCKING_VFS macro. I can produce a patch if it sounds reasonably. Yeah, I think that makes a lot of sense. See attached. Please let me know if it is OK for you. -Maxim -- Name: p p Type: Plain Text (text/plain) Encoding: 7bit Description: ASCII C program text -- * * Konstantin Chuguev Francis House * * Application Engineer 112 Hills Road * Tel: +44 1223 302992 Cambridge CB2 1PQ D A N T E WWW: http://www.dante.netUnited Kingdom To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
apart from the numerical value, yes, looks good.
Poul-Henning
In message [EMAIL PROTECTED], Maxim Sobolev writes:
--%--multipart-mixed-boundary-1.97537.999617732--%
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
In message [EMAIL PROTECTED], Maxim Sobolev writes:
In message [EMAIL PROTECTED], Brent Verner writes:
I've done a /cursory/ look over how this v_tag is used. I'm not sure
this is a simple/clean as you propose, since this is used in the
IS_LOCKING_VFS macro, as well as in union_subr.c...
Well, that is just too bad, because IS_LOCKING_VFS is wrong then.
The places which inspect v_tag will have to be changed to use
strcmp() then...
I think that we can add a new vnode flag, say VCANLOCK, so that each
particular VFS can set it if it supports locking, which should allow
to remove pre-defined VFS list from the IS_LOCKING_VFS macro. I can
produce a patch if it sounds reasonably.
Yeah, I think that makes a lot of sense.
See attached. Please let me know if it is OK for you.
-Maxim
--%--multipart-mixed-boundary-1.97537.999617732--%
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Description: ASCII C program text
Content-Disposition: attachment; filename=p
Index: isofs/cd9660/cd9660_vfsops.c
===
RCS file: /home/ncvs/src/sys/isofs/cd9660/cd9660_vfsops.c,v
retrieving revision 1.91
diff -d -u -r1.91 cd9660_vfsops.c
--- isofs/cd9660/cd9660_vfsops.c 2001/05/16 18:04:30 1.91
+++ isofs/cd9660/cd9660_vfsops.c 2001/09/04 15:20:46
@@ -697,6 +697,7 @@
}
MALLOC(ip, struct iso_node *, sizeof(struct iso_node), M_ISOFSNODE,
M_WAITOK | M_ZERO);
+ vp-v_flag |= VLOCKABLE;
lockinit(vp-v_lock, PINOD, isonode, 0, 0);
/*
* ISOFS uses stdlock and can share lock structure
Index: ufs/ffs/ffs_vfsops.c
===
RCS file: /home/ncvs/src/sys/ufs/ffs/ffs_vfsops.c,v
retrieving revision 1.157
diff -d -u -r1.157 ffs_vfsops.c
--- ufs/ffs/ffs_vfsops.c 2001/06/28 22:21:27 1.157
+++ ufs/ffs/ffs_vfsops.c 2001/09/04 15:21:25
@@ -1172,6 +1172,7 @@
return (error);
}
bzero((caddr_t)ip, sizeof(struct inode));
+ vp-v_flag |= VLOCKABLE;
/*
* FFS supports lock sharing in the stack of vnodes
*/
Index: ufs/ifs/ifs_vfsops.c
===
RCS file: /home/ncvs/src/sys/ufs/ifs/ifs_vfsops.c,v
retrieving revision 1.6
diff -d -u -r1.6 ifs_vfsops.c
--- ufs/ifs/ifs_vfsops.c 2001/04/25 07:07:51 1.6
+++ ufs/ifs/ifs_vfsops.c 2001/09/04 15:21:25
@@ -217,6 +217,7 @@
return (error);
}
bzero((caddr_t)ip, sizeof(struct inode));
+ vp-v_flag |= VLOCKABLE;
/*
* IFS supports lock sharing in the stack of vnodes
*/
Index: nfs/nfs_node.c
===
RCS file: /home/ncvs/src/sys/nfs/nfs_node.c,v
retrieving revision 1.49
diff -d -u -r1.49 nfs_node.c
--- nfs/nfs_node.c 2001/05/01 08:13:14 1.49
+++ nfs/nfs_node.c 2001/09/04 15:21:25
@@ -232,6 +232,7 @@
}
vp = nvp;
bzero((caddr_t)np, sizeof *np);
+ vp-v_flag |= VLOCKABLE;
vp-v_data = np;
np-n_vnode = vp;
/*
Index: sys/vnode.h
===
RCS file: /home/ncvs/src/sys/sys/vnode.h,v
retrieving revision 1.154
diff -d -u -r1.154 vnode.h
--- sys/vnode.h2001/08/27 06:09:55 1.154
+++ sys/vnode.h2001/09/04 15:21:25
@@ -175,6 +175,7 @@
/* open for business 0x10 */
#define VONWORKLST 0x20 /* On syncer work-list */
#define VMOUNT 0x40 /* Mount in progress */
+#define VLOCKABLE 0x60 /* vnode supports locking */
/*
* Vnode attributes. A field value of VNOVAL represents a field whose value
@@ -433,12 +434,7 @@
/*
* [dfr] Kludge until I get around to fixing all the vfs locking.
*/
-#define IS_LOCKING_VFS(vp)((vp)-v_tag == VT_UFS \
- || (vp)-v_tag == VT_NFS \
- || (vp)-v_tag == VT_LFS \
- || (vp)-v_tag == VT_ISOFS \
- || (vp)-v_tag == VT_MSDOSFS \
- || (vp)-v_tag == VT_DEVFS)
+#define IS_LOCKING_VFS(vp)((vp)-v_flag VLOCKABLE)
#define ASSERT_VOP_LOCKED(vp, str)\
do { \
Index: fs/devfs/devfs_vnops.c
===
RCS file: /home/ncvs/src/sys/fs/devfs/devfs_vnops.c,v
retrieving revision 1.27
diff -d -u -r1.27 devfs_vnops.c
---
Re: Junior Kernel Hacker task: improve vnode-v_tag
Hi Maxim, Perhaps you meant: diff -d -u -r1.154 vnode.h --- sys/vnode.h 2001/08/27 06:09:55 1.154 +++ sys/vnode.h 2001/09/04 15:21:25 @@ -175,6 +175,7 @@ /* open for business 0x10 */ #defineVONWORKLST 0x20 /* On syncer work-list */ #defineVMOUNT 0x40 /* Mount in progress */ +#define VLOCKABLE 0x60 /* vnode supports locking */ ...should be +#define VLOCKABLE 0x80 /* vnode supports locking */ Indeed. Thank you for pointing out! -Maxim Maxim Sobolev wrote: In message [EMAIL PROTECTED], Maxim Sobolev writes: In message [EMAIL PROTECTED], Brent Verner writes: I've done a /cursory/ look over how this v_tag is used. I'm not sure this is a simple/clean as you propose, since this is used in the IS_LOCKING_VFS macro, as well as in union_subr.c... Well, that is just too bad, because IS_LOCKING_VFS is wrong then. The places which inspect v_tag will have to be changed to use strcmp() then... I think that we can add a new vnode flag, say VCANLOCK, so that each particular VFS can set it if it supports locking, which should allow to remove pre-defined VFS list from the IS_LOCKING_VFS macro. I can produce a patch if it sounds reasonably. Yeah, I think that makes a lot of sense. See attached. Please let me know if it is OK for you. -Maxim -- Name: p p Type: Plain Text (text/plain) Encoding: 7bit Description: ASCII C program text -- * * Konstantin Chuguev Francis House * * Application Engineer 112 Hills Road * Tel: +44 1223 302992 Cambridge CB2 1PQ D A N T E WWW: http://www.dante.netUnited Kingdom To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
On Tue, 4 Sep 2001, Maxim Sobolev wrote: The places which inspect v_tag will have to be changed to use strcmp() then... I think that we can add a new vnode flag, say VCANLOCK, so that each particular VFS can set it if it supports locking, which should allow to remove pre-defined VFS list from the IS_LOCKING_VFS macro. I can produce a patch if it sounds reasonably. I'm not sure if this a right thing to do. Under SMPng each filesystem is ought to implement correct vnode locking, i.e. vop_nolock() and friends shouldn't exist. -- Boris Popov http://rbp.euro.ru To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
In message [EMAIL PROTECTED], Brent Verner writes: On 04 Sep 2001 at 10:36 (+0200), Poul-Henning Kamp wrote: | | Assignment: | | The v_tag element in struct vnode is a debugging aid, but unfortunately | it is implemented in a way which means that adding a filesystem means | modifying the definition in sys/vnode.h. | | Convert the v_tag to an const char * and have the filesystems put | their name in there instead. | | The v_tag has been abused a few places, easily recognizable by the fact | that the kernel should never inspect the value of v_tag. | These places should be easily changeable to use the new representation. | Please mark them with a big fat /*XXX: ABUSE OF v_tag */ comment. #include newbie_kernel_hacker_warning.h I've done a /cursory/ look over how this v_tag is used. I'm not sure this is a simple/clean as you propose, since this is used in the IS_LOCKING_VFS macro, as well as in union_subr.c... Well, that is just too bad, because IS_LOCKING_VFS is wrong then. The places which inspect v_tag will have to be changed to use strcmp() then... I think that we can add a new vnode flag, say VCANLOCK, so that each particular VFS can set it if it supports locking, which should allow to remove pre-defined VFS list from the IS_LOCKING_VFS macro. I can produce a patch if it sounds reasonably. -Maxim To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
In message [EMAIL PROTECTED], Maxim Sobolev writes: In message [EMAIL PROTECTED], Brent Verner writes: On 04 Sep 2001 at 10:36 (+0200), Poul-Henning Kamp wrote: | | Assignment: | | The v_tag element in struct vnode is a debugging aid, but unfortunately | it is implemented in a way which means that adding a filesystem means | modifying the definition in sys/vnode.h. | | Convert the v_tag to an const char * and have the filesystems put | their name in there instead. | | The v_tag has been abused a few places, easily recognizable by the fact | that the kernel should never inspect the value of v_tag. | These places should be easily changeable to use the new representation. | Please mark them with a big fat /*XXX: ABUSE OF v_tag */ comment. #include newbie_kernel_hacker_warning.h I've done a /cursory/ look over how this v_tag is used. I'm not sure this is a simple/clean as you propose, since this is used in the IS_LOCKING_VFS macro, as well as in union_subr.c... Well, that is just too bad, because IS_LOCKING_VFS is wrong then. The places which inspect v_tag will have to be changed to use strcmp() then... I think that we can add a new vnode flag, say VCANLOCK, so that each particular VFS can set it if it supports locking, which should allow to remove pre-defined VFS list from the IS_LOCKING_VFS macro. I can produce a patch if it sounds reasonably. Yeah, I think that makes a lot of sense. -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
On Tue, 4 Sep 2001, Maxim Sobolev wrote: [neither Maxim Sobolev nor Brent Verner wrote] In message [EMAIL PROTECTED], Brent Verner writes: #include newbie_kernel_hacker_warning.h I've done a /cursory/ look over how this v_tag is used. I'm not sure this is a simple/clean as you propose, since this is used in the IS_LOCKING_VFS macro, as well as in union_subr.c... Well, that is just too bad, because IS_LOCKING_VFS is wrong then. The places which inspect v_tag will have to be changed to use strcmp() then... I think that we can add a new vnode flag, say VCANLOCK, so that each particular VFS can set it if it supports locking, which should allow to remove pre-defined VFS list from the IS_LOCKING_VFS macro. I can produce a patch if it sounds reasonably. Since IS_LOCKING_VFS() is a temporary kludge, the correct fix is to remove it (after fixing whatever it was for), not to add more cruft. Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker task: improve vnode-v_tag
apart from the numerical value, yes, looks good.
Ok, please find the final patch attached. Dare I say that it looks really
ugly?
I'm looking forward for your comments.
-Maxim
Poul-Henning
In message [EMAIL PROTECTED], Maxim Sobolev writes:
--%--multipart-mixed-boundary-1.97537.999617732--%
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
In message [EMAIL PROTECTED], Maxim Sobolev writes:
In message [EMAIL PROTECTED], Brent Verner writes:
I've done a /cursory/ look over how this v_tag is used. I'm not sure
this is a simple/clean as you propose, since this is used in the
IS_LOCKING_VFS macro, as well as in union_subr.c...
Well, that is just too bad, because IS_LOCKING_VFS is wrong then.
The places which inspect v_tag will have to be changed to use
strcmp() then...
I think that we can add a new vnode flag, say VCANLOCK, so that each
particular VFS can set it if it supports locking, which should allow
to remove pre-defined VFS list from the IS_LOCKING_VFS macro. I can
produce a patch if it sounds reasonably.
Yeah, I think that makes a lot of sense.
See attached. Please let me know if it is OK for you.
-Maxim
--%--multipart-mixed-boundary-1.97537.999617732--%
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Description: ASCII C program text
Content-Disposition: attachment; filename=p
Index: isofs/cd9660/cd9660_vfsops.c
===
RCS file: /home/ncvs/src/sys/isofs/cd9660/cd9660_vfsops.c,v
retrieving revision 1.91
diff -d -u -r1.91 cd9660_vfsops.c
--- isofs/cd9660/cd9660_vfsops.c 2001/05/16 18:04:30 1.91
+++ isofs/cd9660/cd9660_vfsops.c 2001/09/04 15:20:46
@@ -697,6 +697,7 @@
}
MALLOC(ip, struct iso_node *, sizeof(struct iso_node), M_ISOFSNODE,
M_WAITOK | M_ZERO);
+vp-v_flag |= VLOCKABLE;
lockinit(vp-v_lock, PINOD, isonode, 0, 0);
/*
* ISOFS uses stdlock and can share lock structure
Index: ufs/ffs/ffs_vfsops.c
===
RCS file: /home/ncvs/src/sys/ufs/ffs/ffs_vfsops.c,v
retrieving revision 1.157
diff -d -u -r1.157 ffs_vfsops.c
--- ufs/ffs/ffs_vfsops.c 2001/06/28 22:21:27 1.157
+++ ufs/ffs/ffs_vfsops.c 2001/09/04 15:21:25
@@ -1172,6 +1172,7 @@
return (error);
}
bzero((caddr_t)ip, sizeof(struct inode));
+vp-v_flag |= VLOCKABLE;
/*
* FFS supports lock sharing in the stack of vnodes
*/
Index: ufs/ifs/ifs_vfsops.c
===
RCS file: /home/ncvs/src/sys/ufs/ifs/ifs_vfsops.c,v
retrieving revision 1.6
diff -d -u -r1.6 ifs_vfsops.c
--- ufs/ifs/ifs_vfsops.c 2001/04/25 07:07:51 1.6
+++ ufs/ifs/ifs_vfsops.c 2001/09/04 15:21:25
@@ -217,6 +217,7 @@
return (error);
}
bzero((caddr_t)ip, sizeof(struct inode));
+vp-v_flag |= VLOCKABLE;
/*
* IFS supports lock sharing in the stack of vnodes
*/
Index: nfs/nfs_node.c
===
RCS file: /home/ncvs/src/sys/nfs/nfs_node.c,v
retrieving revision 1.49
diff -d -u -r1.49 nfs_node.c
--- nfs/nfs_node.c 2001/05/01 08:13:14 1.49
+++ nfs/nfs_node.c 2001/09/04 15:21:25
@@ -232,6 +232,7 @@
}
vp = nvp;
bzero((caddr_t)np, sizeof *np);
+vp-v_flag |= VLOCKABLE;
vp-v_data = np;
np-n_vnode = vp;
/*
Index: sys/vnode.h
===
RCS file: /home/ncvs/src/sys/sys/vnode.h,v
retrieving revision 1.154
diff -d -u -r1.154 vnode.h
--- sys/vnode.h 2001/08/27 06:09:55 1.154
+++ sys/vnode.h 2001/09/04 15:21:25
@@ -175,6 +175,7 @@
/* open for business0x10 */
#define VONWORKLST 0x20 /* On syncer work-list */
#define VMOUNT 0x40 /* Mount in progress */
+#define VLOCKABLE 0x60 /* vnode supports locking */
/*
* Vnode attributes. A field value of VNOVAL represents a field whose value
@@ -433,12 +434,7 @@
/*
* [dfr] Kludge until I get around to fixing all the vfs locking.
*/
-#define IS_LOCKING_VFS(vp) ((vp)-v_tag == VT_UFS \
- || (vp)-v_tag == VT_NFS \
- || (vp)-v_tag == VT_LFS \
- || (vp)-v_tag == VT_ISOFS \
- || (vp)-v_tag == VT_MSDOSFS \
- || (vp)-v_tag == VT_DEVFS)
+#define IS_LOCKING_VFS(vp) ((vp)-v_flag VLOCKABLE)
#define ASSERT_VOP_LOCKED(vp, str) \
do {\
Index: fs/devfs/devfs_vnops.c
