Re: Proposed patch to fix VN device (again)
FYI for the list; this has been fixed and committed now. Another problem in vn when backed with swap was found and fixed too. Matthew Dillon wrote: : :Matthew Dillon wrote: :[..] : And, in anycase, I am not going to spend hours putting together a long : involved patch when a simple short patch suffices. If you want to : spend the time to come up with your own patch (that doesn't screw the : pooch in regards to cluster performance!), then I'm all ears, otherwis e : this patch is what is going to go in for the 4.0 release. : :Please hold fire for a bit longer. I have (what I think is) a much cleaner :fix in mind. There is no urgency here, a couple of hours isn't going to :make the slightest difference to the big picture. : :On a side note, as the original author of the changes that lead to this, I :would have thought it would have been appropriate to ask ME directly first :before going public. : :Cheers, :-Peter Your fix looks pretty good, Peter. I'll run it in and test it. I only wish you had come up with it before Poul started going off the deep end. Cheers, -Peter -- Peter Wemm - [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
At 8:12 PM +0800 1999/12/28, Peter Wemm wrote: FYI for the list; this has been fixed and committed now. Another problem in vn when backed with swap was found and fixed too. Was the vn problem an issue for 3.x as well? If so, is there any chance of getting this mfc'ed any time soon? -- These are my opinions -- not to be taken as official Skynet policy |o| Brad Knowles, [EMAIL PROTECTED]Belgacom Skynet NV/SA |o| |o| Systems Architect, News FTP Admin Rue Col. Bourg, 124 |o| |o| Phone/Fax: +32-2-706.11.11/12.49 B-1140 Brussels |o| |o| http://www.skynet.be Belgium |o| \/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/ Unix is like a wigwam -- no Gates, no Windows, and an Apache inside. Unix is very user-friendly. It's just picky who its friends are. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
Brad Knowles wrote: At 8:12 PM +0800 1999/12/28, Peter Wemm wrote: FYI for the list; this has been fixed and committed now. Another problem in vn when backed with swap was found and fixed too. Was the vn problem an issue for 3.x as well? If so, is there any chance of getting this mfc'ed any time soon? No, 3.x doesn't support swap-backed vn devices. Cheers, -Peter To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
In message [EMAIL PROTECTED], Matthew Dillon writes: Sigh. Ok, I've fixed the VN device. Again. It looks like the removal of /dev/drum removed a little too much. We need the device infrastructure to support the VN device's use of swap backing store. This patch below is a commit candidate. It could use a review, then I'll commit it. Could you please explain why you need this ? -- Poul-Henning Kamp FreeBSD coreteam member [EMAIL PROTECTED] "Real hackers run -current on their laptop." FreeBSD -- It will take a long time before progress goes too far! To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
: :In message [EMAIL PROTECTED], Matthew Dillon writes: : :Sigh. Ok, I've fixed the VN device. Again. It looks like the removal of :/dev/drum removed a little too much. We need the device infrastructure :to support the VN device's use of swap backing store. : :This patch below is a commit candidate. It could use a review, then I'll :commit it. : :Could you please explain why you need this ? : :-- :Poul-Henning Kamp FreeBSD coreteam member :[EMAIL PROTECTED] "Real hackers run -current on their laptop." :FreeBSD -- It will take a long time before progress goes too far! VN calls vm_pager_strategy() which collects I/O in filesystem buffers (bp)'s in order to cluster the I/O, and you cannot initiate I/O on filesystem buffers without a valid b_dev. So, the jist of the problem is that in order to get reasonable performance out of the thing (i.e. not issue 4K I/O's to the disk), we have to use filesystem buffers which means the bp must be properly initialized. -Matt Matthew Dillon [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
In message [EMAIL PROTECTED], Matthew Dillon writes: VN calls vm_pager_strategy() which collects I/O in filesystem buffers (bp)'s in order to cluster the I/O, and you cannot initiate I/O on filesystem buffers without a valid b_dev. So, the jist of the problem is that in order to get reasonable performance out of the thing (i.e. not issue 4K I/O's to the disk), we have to use filesystem buffers which means the bp must be properly initialized. vm_pager_strategy() branches out based on the object type and ends up in swap_pager_strategy() which only uses b_dev after calling swstrategy() then only in a diagnostic printf which we had better never reach. swstrategy() which initializes bp-b_dev to the right physical device. I'm afraid I don't see why or where we would need the bogus /dev/drum dev_t. Furthermore, since we have already clustered above the VN device, I seriously doubt the utility of doing it again between VN and the swapdevice. -- Poul-Henning Kamp FreeBSD coreteam member [EMAIL PROTECTED] "Real hackers run -current on their laptop." FreeBSD -- It will take a long time before progress goes too far! To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
:In message [EMAIL PROTECTED], Matthew Dillon writes: : :VN calls vm_pager_strategy() which collects I/O in filesystem buffers :(bp)'s in order to cluster the I/O, and you cannot initiate I/O on :filesystem buffers without a valid b_dev. : :So, the jist of the problem is that in order to get reasonable performance :out of the thing (i.e. not issue 4K I/O's to the disk), we have to use :filesystem buffers which means the bp must be properly initialized. : :vm_pager_strategy() branches out based on the object type and ends up :in swap_pager_strategy() which only uses b_dev after calling swstrategy() :then only in a diagnostic printf which we had better never reach. : :swstrategy() which initializes bp-b_dev to the right physical device. : :I'm afraid I don't see why or where we would need the bogus /dev/drum dev_t. : :Furthermore, since we have already clustered above the VN device, :I seriously doubt the utility of doing it again between VN and the :swapdevice. : :-- :Poul-Henning Kamp FreeBSD coreteam member :[EMAIL PROTECTED] "Real hackers run -current on their laptop." :FreeBSD -- It will take a long time before progress goes too far! Umm. Poul, I know you don't like the idea but you obviously do not understand the code in question. First, you are confusing the underlying swap devices that we swapon on with the parent swap device that controls them. The parent swap device needs to have a dev_t - it does not currently have one, and this is the entry point that the buffers are going into. The underlying swap devices *already* have a dev_t. My kernel crashes on a NULL bp-b_dev. In fact, it also crashes on a NULL vp-v_rdev. Without the patch. So if you are trying to say that b_dev is being magically set I gotta tell you that my kernel dumps say otherwise. Second, the clustering done above the VN device is done by the filesystem and has no understanding of whether the underlying media controlled by the VN device can itself be clustered in the same way. When using swap as backing store what may appear to be clusterable by the filesystem may actually *NOT* be clusterable when you get into the swap device due to potentially non-contiguous mappings as well as border-crossings between interleaved swap devices. -Matt Matthew Dillon [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
In message [EMAIL PROTECTED], Matthew Dillon writes: First, you are confusing the underlying swap devices that we swapon on with the parent swap device that controls them. The parent swap device needs to have a dev_t - it does not currently have one, and this is the entry point that the buffers are going into. The underlying swap devices *already* have a dev_t. My kernel crashes on a NULL bp-b_dev. In fact, it also crashes on a NULL vp-v_rdev. And where in the code does this crash happen ? Second, the clustering done above the VN device is done by the filesystem and has no understanding of whether the underlying media controlled by the VN device can itself be clustered in the same way. When using swap as backing store what may appear to be clusterable by the filesystem may actually *NOT* be clusterable when you get into the swap device due to potentially non-contiguous mappings as well as border-crossings between interleaved swap devices. So you need to cluster all the way through the VN device ? Obviously clustering on just one side will not buy you anything. Tough luck... It sounds more and more like the mistake here is for VN to have a specfs VOP vector on top, I think it needs its own VOP vector so it can get hold of the VOP_BMAP function... -- Poul-Henning Kamp FreeBSD coreteam member [EMAIL PROTECTED] "Real hackers run -current on their laptop." FreeBSD -- It will take a long time before progress goes too far! To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
:In message [EMAIL PROTECTED], Matthew Dillon writes: : :First, you are confusing the underlying swap devices that we swapon on :with the parent swap device that controls them. The parent swap device :needs to have a dev_t - it does not currently have one, and this is :the entry point that the buffers are going into. The underlying swap :devices *already* have a dev_t. My kernel crashes on a NULL bp-b_dev. :In fact, it also crashes on a NULL vp-v_rdev. : :And where in the code does this crash happen ? #9 0xc023c5b3 in trap () #10 0xc014b862 in devsw () #11 0xc017fa4a in vn_isdisk () #12 0xc018e1f3 in spec_strategy () #13 0xc020eb42 in flushchainbuf () #14 0xc0202e1e in swap_pager_strategy () #15 0xc020e675 in vm_pager_strategy () #9 0xc023c5e3 in trap () #10 0xc014b862 in devsw () #11 0xc018e26e in spec_strategy () #12 0xc020eb72 in flushchainbuf () #13 0xc0202e4e in swap_pager_strategy () #14 0xc020e6a5 in vm_pager_strategy () #9 0xc023c5c3 in trap () #10 0xc018e23f in spec_strategy () #11 0xc020eb52 in flushchainbuf () #12 0xc0202e2e in swap_pager_strategy () #13 0xc020e685 in vm_pager_strategy () :Second, the clustering done above the VN device is done by the :filesystem and has no understanding of whether the underlying media :controlled by the VN device can itself be clustered in the same way. :When using swap as backing store what may appear to be clusterable :by the filesystem may actually *NOT* be clusterable when you get into :the swap device due to potentially non-contiguous mappings as well :as border-crossings between interleaved swap devices. : :So you need to cluster all the way through the VN device ? Obviously :clustering on just one side will not buy you anything. Tough luck... : :It sounds more and more like the mistake here is for VN to have a :specfs VOP vector on top, I think it needs its own VOP vector so :it can get hold of the VOP_BMAP function... : :-- :Poul-Henning Kamp FreeBSD coreteam member :[EMAIL PROTECTED] "Real hackers run -current on their laptop." Ummm. The VN device is not a filesystem, it's a 'disk'. The UFS filesystem does not recursively VOP_BMAP down into the underlying media so implementing VOP_BMAP in VN would not help. We could implement our own set of device vectors but it would be rather pointless since specfs's device vectors do everything we want. And, in anycase, I am not going to spend hours putting together a long involved patch when a simple short patch suffices. If you want to spend the time to come up with your own patch (that doesn't screw the pooch in regards to cluster performance!), then I'm all ears, otherwise this patch is what is going to go in for the 4.0 release. -Matt Matthew Dillon [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
Ahh, I see the mistake.
All you need to fix this is to add a new function:
void
flushswchainbuf(struct buf *nbp)
{
if (nbp-b_bcount) {
nbp-b_bufsize = nbp-b_bcount;
if ((nbp-b_flags B_READ) == 0)
nbp-b_dirtyend = nbp-b_bcount;
BUF_KERNPROC(nbp);
swstrategy(nbp);
} else {
biodone(nbp);
}
}
And use this instead of flushchainbuf() in swap_pager_strategy().
--
Poul-Henning Kamp FreeBSD coreteam member
[EMAIL PROTECTED] "Real hackers run -current on their laptop."
FreeBSD -- It will take a long time before progress goes too far!
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
:
:
:Ahh, I see the mistake.
:
:All you need to fix this is to add a new function:
:
: void
: flushswchainbuf(struct buf *nbp)
: {
: if (nbp-b_bcount) {
: nbp-b_bufsize = nbp-b_bcount;
: if ((nbp-b_flags B_READ) == 0)
: nbp-b_dirtyend = nbp-b_bcount;
: BUF_KERNPROC(nbp);
: swstrategy(nbp);
: } else {
: biodone(nbp);
: }
: }
:
:And use this instead of flushchainbuf() in swap_pager_strategy().
:
:--
:Poul-Henning Kamp FreeBSD coreteam member
:[EMAIL PROTECTED] "Real hackers run -current on their laptop."
Uh... no. The chain buffer routines are supposed to be generic. In fact,
all the filesystem buffer cache I/O routines are supposed to be generic.
There is no way I'm going to pollute them to special case swap.
-Matt
Matthew Dillon
[EMAIL PROTECTED]
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
In message [EMAIL PROTECTED], Matthew Dillon writes:
:
:Ahh, I see the mistake.
:
:All you need to fix this is to add a new function:
:
: void
: flushswchainbuf(struct buf *nbp)
: {
: if (nbp-b_bcount) {
: nbp-b_bufsize = nbp-b_bcount;
: if ((nbp-b_flags B_READ) == 0)
: nbp-b_dirtyend = nbp-b_bcount;
: BUF_KERNPROC(nbp);
: swstrategy(nbp);
: } else {
: biodone(nbp);
: }
: }
:
:And use this instead of flushchainbuf() in swap_pager_strategy().
:
:--
:Poul-Henning Kamp FreeBSD coreteam member
:[EMAIL PROTECTED] "Real hackers run -current on their laptop."
Uh... no. The chain buffer routines are supposed to be generic. In fact,
all the filesystem buffer cache I/O routines are supposed to be generic.
There is no way I'm going to pollute them to special case swap.
Ok, fair enough: Then stop abusing struct buf for swap device access :-)
If you want to abuse struct buf for swap device access, you will need
to special case it as applicable.
--
Poul-Henning Kamp FreeBSD coreteam member
[EMAIL PROTECTED] "Real hackers run -current on their laptop."
FreeBSD -- It will take a long time before progress goes too far!
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
::-- ::Poul-Henning Kamp FreeBSD coreteam member ::[EMAIL PROTECTED] "Real hackers run -current on their laptop." : :Uh... no. The chain buffer routines are supposed to be generic. In fact, :all the filesystem buffer cache I/O routines are supposed to be generic. :There is no way I'm going to pollute them to special case swap. : :Ok, fair enough: Then stop abusing struct buf for swap device access :-) : :If you want to abuse struct buf for swap device access, you will need :to special case it as applicable. : :-- :Poul-Henning Kamp FreeBSD coreteam member :[EMAIL PROTECTED] "Real hackers run -current on their laptop." Huh? Look Poul, I don't know what your problem is but I'm through playing around with you. You aren't making any sense and, frankly my dear sir, the buffer cache, vm_pager interface, VN device, and vm_pager_strategy subsystem is my ball of wax, not yours... I don't need your permission and I certainly don't need your help if this is all you can come up with. I'm not going to twist up all that wonderful clean code just to avoid making the swapper a real device. After all, making it a real device is only a few simple lines of code - simple and straightforward. -Matt Matthew Dillon [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
In message [EMAIL PROTECTED], Matthew Dillon writes: Huh? Look Poul, I don't know what your problem is but I'm through playing around with you. You aren't making any sense and, frankly my dear sir, the buffer cache, vm_pager interface, VN device, and vm_pager_strategy subsystem is my ball of wax, not yours... Sorry, not so (and you've been told plenty many times already). You should fix the swap-pager to remove this last smoldering assumption about the existence of /dev/drum, rather than obfuscate the rest of the system by adding it back. I know it means more work for you, but we're not here to cut corners, we're here to get a better system. We don't get a better system if you keep pushing the dirty bits into obscure corners of the kernel rather than fix them where they need fixed. I don't need your permission and I certainly don't need your help if this is all you can come up with. I objected to your patch on the grounds that it would be a step backwards, that has nothing to do with getting my permission. can come up with. I'm not going to twist up all that wonderful clean code just to avoid making the swapper a real device. You can call that "wonderful clean code" all you like, it is most certainly not the term I would use for it, and either way the matter of the fact is that struct buf is being abused to do something which it isn't able to do in its current design, and the way to fix that is not to add a hacked up dev_t but to give it special handling where it needs that. And adding a bogus dev_t and a equally bogus devsw does by no means make the swapper "a real device". I still object to your patch. It is a step backwards and it would further obfuscate the swap_pagers code. -- Poul-Henning Kamp FreeBSD coreteam member [EMAIL PROTECTED] "Real hackers run -current on their laptop." FreeBSD -- It will take a long time before progress goes too far! To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
: :In message [EMAIL PROTECTED], Matthew Dillon writes: : :Huh? Look Poul, I don't know what your problem is but I'm through :playing around with you. You aren't making any sense and, frankly my :dear sir, the buffer cache, vm_pager interface, VN device, and :vm_pager_strategy subsystem is my ball of wax, not yours... : :Sorry, not so (and you've been told plenty many times already). : :You should fix the swap-pager to remove this last smoldering :assumption about the existence of /dev/drum, rather than obfuscate :the rest of the system by adding it back. : :I know it means more work for you, but we're not here to cut corners, :we're here to get a better system. We don't get a better system if :you keep pushing the dirty bits into obscure corners of the kernel :rather than fix them where they need fixed. Sorry Poul, but you have no authority in this matter. All the statements you have made in the last hour simply point out, in very clear terms, that you don't know what you are talking about. Instead of simply admiting a mistake or even spending some time to review the code more closely, your solution is instead to continue to make outrageous statements. Frankly, Poul, I strongly, *STRONGLY* recommend that you simply not reply to any of my postings. Not a single one, because I am wholely sick and tired of your superiority complex. This thread was not meant for you, and your presence in it is not welcome. And don't even think about doing an end-run around me and committing your frankly inappropriate 'solution' - I'm going through proper channels to get this in. -Matt To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
In message [EMAIL PROTECTED], Matthew Dillon writes: Sorry Poul, but you have no authority in this matter. All the statements you have made in the last hour simply point out, in very clear terms, that you don't know what you are talking about. Instead of simply admiting a mistake or even spending some time to review the code more closely, your solution is instead to continue to make outrageous statements. The fact that I think the hacks should be kept firmly inside the VM system, rather than spill out into the rest of the kernel is not something I count amongst my mistakes. I have actually spent quite some time looking at the code, both before and again tonight just because of your email. That is probably the best example of a review process which works. Frankly, Poul, I strongly, *STRONGLY* recommend that you simply not reply to any of my postings. This has been my general policy for a long time, but it gets overridden whenever you come up with something which is obviously wrong. (And I find the fact that so many people have adopted this policy disturbing, and sufficient grounds for me to relax my instance of it.) Not a single one, because I am wholely sick and tired of your superiority complex. This thread was not meant for you, and your presence in it is not welcome. And don't even think about doing an end-run around me and committing your frankly inappropriate 'solution' - I'm going through proper channels to get this in. I have all rights I need to answer you when you send an email in public asking for a review. It is my obligation to answer you when I am one of the few people who are not intimidated by neither you nor the code in question and I can see that what you are proposing is plain wrong. So far you have one very experienced kernel programmer objecting to your patch, so yes: you will bloddy have to go through proper channels to resolve this issue. -- Poul-Henning Kamp FreeBSD coreteam member [EMAIL PROTECTED] "Real hackers run -current on their laptop." FreeBSD -- It will take a long time before progress goes too far! To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
I've heard from both of you that you think the other is wrong. This isn't very helpful, however, in finding the correct solution. What I'd like to hear from both of you is the reasons why swap is better as a device, or not. There seems to be some unstated architectural philosophy that needs to be stated before any informed decision can be made about what is the right direction to go in. -DG David Greenman Co-founder/Principal Architect, The FreeBSD Project - http://www.freebsd.org Creator of high-performance Internet servers - http://www.terasolutions.com Pave the road of life with opportunities. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
Matthew Dillon wrote: [..] And, in anycase, I am not going to spend hours putting together a long involved patch when a simple short patch suffices. If you want to spend the time to come up with your own patch (that doesn't screw the pooch in regards to cluster performance!), then I'm all ears, otherwise this patch is what is going to go in for the 4.0 release. Please hold fire for a bit longer. I have (what I think is) a much cleaner fix in mind. There is no urgency here, a couple of hours isn't going to make the slightest difference to the big picture. On a side note, as the original author of the changes that lead to this, I would have thought it would have been appropriate to ask ME directly first before going public. Cheers, -Peter To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
David Greenman wrote:
I've heard from both of you that you think the other is wrong. This isn't
very helpful, however, in finding the correct solution. What I'd like to hear
from both of you is the reasons why swap is better as a device, or not. There
seems to be some unstated architectural philosophy that needs to be stated
before any informed decision can be made about what is the right direction to
go in.
The problem is that swapdev_vp needs to handle VOP_STRATEGY(), and swapdev_vp
is incorrectly being pointed at spec_vnops. Here is a proposed (UNTESTED!)
clean fix:
Index: swap_pager.c
===
RCS file: /home/ncvs/src/sys/vm/swap_pager.c,v
retrieving revision 1.129
diff -u -r1.129 swap_pager.c
--- swap_pager.c1999/11/22 15:27:09 1.129
+++ swap_pager.c1999/12/28 03:22:08
@@ -199,6 +199,32 @@
static daddr_t swp_pager_meta_ctl __P((vm_object_t, vm_pindex_t, int));
/*
+ * Handle a VOP_STRATEGY() on swapdev_vp
+ */
+
+static int
+swapdev_strategy(ap)
+ struct vop_strategy_args /* {
+ struct vnode *a_vp;
+ struct buf *a_bp;
+ } */ *ap;
+{
+ struct buf *bp;
+
+ bp = ap-a_bp;
+ return swstrategy(bp);
+}
+
+vop_t **swapdev_vnodeop_p;
+static struct vnodeopv_entry_desc swapdev_vnodeop_entries[] = {
+ { vop_strategy_desc, (vop_t *) swapdev_strategy },
+ { NULL, NULL }
+};
+static struct vnodeopv_desc swapdev_vnodeop_opv_desc =
+ { swapdev_vnodeop_p, swapdev_vnodeop_entries };
+VNODEOP_SET(swapdev_vnodeop_opv_desc);
+
+/*
* SWP_SIZECHECK() - update swap_pager_full indication
*
* update the swap_pager_almost_full indication and warn when we are
@@ -329,7 +355,7 @@
swhash_mask = n - 1;
- n = getnewvnode(VT_NON, NULL, spec_vnodeop_p, swapdev_vp);
+ n = getnewvnode(VT_NON, NULL, swapdev_vnodeop_p, swapdev_vp);
if (n)
panic("Cannot get vnode for swapdev");
swapdev_vp-v_type = VBLK;
After this, flushchainbuf() on a bp that is bound to swapdev_vp should do
the right thing. There is still no need at all to redirect this via a fake
device.
I'll be testing this shortly, but I wanted to get an alternative in before the
arms race truely turned nuclear.
Cheers,
-Peter
--
Peter Wemm - [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED]
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
: : I've heard from both of you that you think the other is wrong. This isn't :very helpful, however, in finding the correct solution. What I'd like to hear :from both of you is the reasons why swap is better as a device, or not. There :seems to be some unstated architectural philosophy that needs to be stated :before any informed decision can be made about what is the right direction to :go in. : :-DG : :David Greenman :Co-founder/Principal Architect, The FreeBSD Project - http://www.freebsd.org Ok, I'll buy that. Though I will note that VN has been broken for over a month - I found that out about half a month ago and posted the fact, and if Poul is so interested in fixing it he could easily have done so then. I'm the one who has spent all the time tracking it down, patching, and testing it, and frankly that should count for a hellofalot more then nil. Poul frankly does not have a leg to stand on to demand that I work on a 'better' solution when I didn't cause the problem in the first place, nor does he have any right to pollute the VM code to end-run around the issue. It would be tragic if Poul's behavior were allowed to conclude the matter. That said, here is my position, split into a 'history' and my 'reasoning': Previously we had /dev/drum, which didn't work, and the SWAP device was layered around a device and swap I/O was vectored through the device subsystem. One month ago, after discussions, Peter Wemm removed the userland frontend for the swap device, /dev/drum, and at the same time thought it would be a good idea to collapse the VOP_STRATEGY routines (that ran through the SWAP dev_t) into direct calls to swstrategy(). At the time I was ambivalent but since the change seemed to clear out a bunch of cruft I went with it. However, this broke the VN device. I found this out about half a month later (around half a month ago) but at the time was hip deep in NFS work for the 3.4 release and could not address the issue. Now that the NFS work is finished I revisited the now broken VN device to determine what went wrong. It turns out that the VN device indirectly runs through the swap device using several generic filesystem buffer related subsystems. When the swap dev_t was removed, the result was a panic. - The VN device uses vm_pager_strategy() to implement swap backing store. vm_pager_strategy() is a generic mechanism that I introduced into the VM system a number of months ago for two reasons: (1) As a sounding board for giving the vm_pager_*() functions a strategy entry point, to see how well it would work. (2) To implement the swap backing store feature for the VN device. vm_pager_strategy() in turn looks at the pager ID and does a table lookup call through to the swap_pager_strategy() function. This function takes a VM object and a *generic* bp. While the function itself is swap-specific, the design of the code is supposed to be as generic as possible. vm_pager_strategy() may need to cut up the bp into subrequests to deal with clustering issues. It uses the generic 'chainbuf' routines to manage the asynchronous I/O. The 'chainbuf' routines are designed to be as generic as possible.. they take BP's. They do *NOT* and are not intended to be special cased in any way. This is my design for these routines, and even though they are currently used only by the swap_pager_strategy() code I believe the generic nature of the routines must be maintained. I/O sequencing for the swap-backed VN device is thus: VN-vm_pager_strategy-chainbuf-STRATEGY. I advocate that the best solution is to simply undo part of the original commit and restore SWAP's dev_t which was removed a month ago. Poul's solution is considerably more complicated (the patch he offered will not work in all cases, and I've already corrected all his other misstatements that I do not really feel it appropriate to continue to correct his misstatements when all he does with those corrections is use them as a poolboard to make his next set of incorrect statements!). I will tell you why Poul's patch will not work, but I am NOT going to tell Poul because, damn it, he has no right to spend 5 minutes working up a shoddy patch and then try to dictate to me that it should be used over my well tested 8-hours-of-work patch! Poul's patch will not work because flushchainbuf() is not the only chain routine that flushes the buffers. getchainbuf() and waitchainbuf() may *also* flush the buffers if there are already too many chain buffers built up in the linked list. Poul only addressed flushchainbuf(). He obviously didn't bother to read the code carefully and he just as obviously doesn't understand it. And not understanding it I can't imagine
Re: Proposed patch to fix VN device (again)
In message [EMAIL PROTECTED], David Greenman writes: There seems to be some unstated architectural philosophy that needs to be stated before any informed decision can be made about what is the right direction to go in. The underlying problem is of course that struct buf is a conglomerate of "struct block_cache" and "struct io_request", and in the cases where you need only the latter functionality you still have to respect all the conventions of the former. This issue leads to complexity and hacks in all code that needs to splite a io operation: vinum, ccd, swap_pager and disk_slice/label. For this reason the swap-pager has a bogus vnode: (swapdev_vp) to hang buffers from. This vnode even more bogusly uses spec_vnodeop_p for its vop[], and at the far end of that vop[] it used to abuse /dev/drum to get things done. If we want to maintain the presently unused generality of the chainbuf routines (they are currently only used in the swap-pager), then the fix is to add a real vop[] to the bogus swapdev_vp so that VOP_STRATEGY correctly end up in swstrategy() for swap device buffers. If we accept that the chainbuf routines are of limited utility, we can simply s/VOP_STRATEGY/swstrategy/ in the flushchain procedure. The reason why /dev/drum should not be brought back is that it isn't a device and it shouldn't be made one. In particular it should not made one to paste over a bogus vnode, that would be a step backwards. If we want to *really* fix the problem, we should introduce a bop[] function vector on struct buf. Kirk already (almost) did this: struct bio_ops. This bop[] would be where code would find the strategy, bwrite* and other operations on the buffer without knowing if the buffer has a dev_t, a dev_t + a vnode, only a vnode or neither associated with it. This fix is out of scope for the present problem, and certainly not a 4.0 item. This fix will be needed before we can put a non-device backed storage layer under UFS. -- Poul-Henning Kamp FreeBSD coreteam member [EMAIL PROTECTED] "Real hackers run -current on their laptop." FreeBSD -- It will take a long time before progress goes too far! To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
Frankly, Poul, I strongly, *STRONGLY* recommend that you simply not reply to any of my postings. Not a single one, because I am wholely sick and tired of your superiority complex. This thread was not meant This is unwontedly personal and has no place in a public mailing list. Please keep your replies confined to technical content (or your concerns about the lack thereof) and keep always in mind that getting emotional serves only to generate heat when what's needed is light. - Jordan To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
It makes no sense whatsoever to spend hours or days reworking a major subsystem *just* so the swap device can do without a dev_t. I don't really give a damn about /dev/drum -- I said before and I will say again that we can leave it out. But we need to give the swap device its's dev_t back to fix VN because no matter what you believe the *right* thing is to do, (A) nobody has time to rework a major subsystem at this time, and (B) VN was broken when the dev_t was removed, the dev_t should be added back in until someone comes up with a better overall solution. I also strongly believe that polluting *OTHER* subsystems (the buffer chaining code) just to avoid creating a dev_t for SWAP makes even less sense. I do not have the inclination to spend huge amounts of my time reworking the VOP or filesystem buffer subsystem because someone isn't willing to spend a tiny amount of code to partially back out a patch that broke something (VN) in the system, that someone also not themselves willing to spend the time required to rework the system properly. It's that simple. It makes no sense to impose such a requirement on someone else to fix a problem that they did not create. Now I would like to make a clarification: The commit we are talking about was actually one Peter did with the approval of everyone, including me. At the end of November.However, nobody including me realized that the second part of his commit, which was an optimization to remove the 'cruft' of the SWAP device, broke VN. If we had known then that this optimization would break VN it would *NOT* have gone in until the VN problem was fixed. -Matt To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
Peter Wemm wrote: David Greenman wrote: I've heard from both of you that you think the other is wrong. This isn' t very helpful, however, in finding the correct solution. What I'd like to he ar from both of you is the reasons why swap is better as a device, or not. The re seems to be some unstated architectural philosophy that needs to be stated before any informed decision can be made about what is the right direction to go in. The problem is that swapdev_vp needs to handle VOP_STRATEGY(), and swapdev_vp is incorrectly being pointed at spec_vnops. Here is a proposed (UNTESTED!) clean fix: This is missing a vop_default entry, so it will panic. But as a proof of concept it still stands. Cheers, -Peter To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
: :Matthew Dillon wrote: :[..] : And, in anycase, I am not going to spend hours putting together a long : involved patch when a simple short patch suffices. If you want to : spend the time to come up with your own patch (that doesn't screw the : pooch in regards to cluster performance!), then I'm all ears, otherwise : this patch is what is going to go in for the 4.0 release. : :Please hold fire for a bit longer. I have (what I think is) a much cleaner :fix in mind. There is no urgency here, a couple of hours isn't going to :make the slightest difference to the big picture. : :On a side note, as the original author of the changes that lead to this, I :would have thought it would have been appropriate to ask ME directly first :before going public. : :Cheers, :-Peter Your fix looks pretty good, Peter. I'll run it in and test it. I only wish you had come up with it before Poul started going off the deep end. -Matt Matthew Dillon [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposed patch to fix VN device (again)
This is unwontedly personal and has no place in a public mailing list. Please keep your replies confined to technical content (or your concerns about the lack thereof) and keep always in mind that getting emotional serves only to generate heat when what's needed is light. - Jordan And it undermines even the strongest of technical arguments. -Kip To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
