2.4.19pre3aa2
URL:
http://www.us.kernel.org/pub/linux/kernel/people/andrea/kernels/v2.4/2.4.19pre3aa2.gz http://www.us.kernel.org/pub/linux/kernel/people/andrea/kernels/v2.4/2.4.19pre3aa2/
VM alone:
http://www.us.kernel.org/pub/linux/kernel/people/andrea/patches/v2.4/2.4.19pre3/vm-32
Changelog:
Only in 2.4.19pre3aa1: 00_lcall_trace-1
Obsoleted by mainline (thanks to Christoph Hellwig for the reminder).
Only in 2.4.19pre3aa2: 00_loop-IV-API-hvr-1
Make the IV API not to be in function of the blkdev of the underlying fs, so you can copy your cryptoloop around without risking to lose data. This breaks the on-disk format of some encrypted transfer module though, if you don't like it please discuss it here in CC with Herbert Valerio Riedel <[email protected]>, the patch
is from him. I think writing a converter in place of the loop data would be the preferred solution if needed. It could be done in a way to link transparently with the source of the kernel modules.
Only in 2.4.19pre3aa2: 00_multipath-routing-smp-1
SMP race fix.
Only in 2.4.19pre3aa2: 00_zlib-1
the zlib fix.
Only in 2.4.19pre3aa1: 10_nfs-o_direct-4 Only in 2.4.19pre3aa2: 10_nfs-o_direct-5 Only in 2.4.19pre3aa1: 10_reiserfs-o_direct-1
Merged together for consistency, suggested by Christoph Hellwig.
Only in 2.4.19pre3aa1: 10_vm-31 Only in 2.4.19pre3aa2: 10_vm-32
Fixed ext3 deadlock and "theorical" mainline SMP race for some arch.
Only in 2.4.19pre3aa2: 21_pte-highmem-f00f-1
vmalloc called before smp_init was an hack, right way is to use fixmap. CONFIG_M686 doesn't mean much these days, but it's ok and probably most vendors will use it for the smp kernels, so it will save 4096 of the vmalloc space. I just didn't wanted to clobber the code with || CONFIG_K7 || CONFIG_... | ... given all the other f00f stuff is also conditional only to M686 and probably nobody bothered to compile it out for my same reason (if somebody worries about the few kbytes of bytecode and the 4096 bytes of vmalloc virtual address space feel free to send me a patch :).
This allows booting P5 SMP with pte-highmem.
Only in 2.4.19pre3aa1: 70_xfs-6.gz Only in 2.4.19pre3aa2: 70_xfs-7.gz
Rediffed for the vm deadlock fix, also clear PG_Launder _before_ unlocking the buffer in write_buffer_locked().
Andrea
Quoted 10 lines · Andrea Arcangeli
Only in 2.4.19pre3aa2: 21_pte-highmem-f00f-1
vmalloc called before
smp_initwas an hack, right way is to use fixmap.CONFIG_M686doesn't mean much these days, but it's ok and probably most vendors will use it for the smp kernels, so it will save 4096 of the vmalloc space. I just didn't wanted to clobber the code with ||CONFIG_K7||CONFIG_... | ... given all the other f00f stuff is also conditional only to M686 and probably nobody bothered to compile it out for my same reason
Brian Gerst had a patch a few months back to introduce a CONFIG_F00F if a relevant CONFIG_Mxxx was chosen[1]. It never got applied anywhere, but makes more sense than the CONFIG_M686 we currently use.
[1] 386/486/586. With addition of my Vendor choice menu, we could even further narrow it down to Intel only.
Signature · 2 lines
| Dave Jones. http://www.codemonkey.org.uk | SuSE Labs
Quoted 12 lines · Dave Jones
Quoted 10 lines · Andrea Arcangeli
Only in 2.4.19pre3aa2: 21_pte-highmem-f00f-1
vmalloc called before
smp_initwas an hack, right way is to use fixmap.CONFIG_M686doesn't mean much these days, but it's ok and probably most vendors will use it for the smp kernels, so it will save 4096 of the vmalloc space. I just didn't wanted to clobber the code with ||CONFIG_K7||CONFIG_... | ... given all the other f00f stuff is also conditional only to M686 and probably nobody bothered to compile it out for my same reason
Quoted 6 lines
Brian Gerst had a patch a few months back to introduce a
CONFIG_F00Fif a relevantCONFIG_Mxxxwas chosen[1]. It never got applied anywhere, but makes more sense than theCONFIG_M686we currently use.[1] 386/486/586. With addition of my Vendor choice menu, we could even further narrow it down to Intel only.
Since vendors (and consultants) like to build a single kernel for use on
multiple machines, it would be nice if this could be done by some init code (released) and a module. I don't know what the overhead would be, perhaps the runtime code is so small it's not worth doing. Does that mean it's not worth doing the option either? It certainly would seen desirable to check for the F00F bug and if the code to handle it was not present refuse to boot right away.
The code actually looks so small as to be unworthy of an option, given
that many people would set it off not knowing was it was much less whether they needed it. This is not like a missing FPU where you can do a graceful reject of the instructions, if you have the bug and not the fix you are vulnerable to sudden total failures, correct?
Signature · 3 lines
bill davidsen <[email protected]> CTO, TMR Associates, Inc Doing interesting things with little computers since 1979.
Quoted 18 lines · Bill Davidsen
Quoted 16 lines · Dave Jones
Quoted 10 lines · Andrea Arcangeli
Only in 2.4.19pre3aa2: 21_pte-highmem-f00f-1
vmalloc called before
smp_initwas an hack, right way is to use fixmap.CONFIG_M686doesn't mean much these days, but it's ok and probably most vendors will use it for the smp kernels, so it will save 4096 of the vmalloc space. I just didn't wanted to clobber the code with ||CONFIG_K7||CONFIG_... | ... given all the other f00f stuff is also conditional only to M686 and probably nobody bothered to compile it out for my same reasonBrian Gerst had a patch a few months back to introduce a
CONFIG_F00Fif a relevantCONFIG_Mxxxwas chosen[1]. It never got applied anywhere, but makes more sense than theCONFIG_M686we currently use.
Quoted 7 lines
Quoted 2 lines
[1] 386/486/586. With addition of my Vendor choice menu, we could even further narrow it down to Intel only.
Since vendors (and consultants) like to build a single kernel for use on
multiple machines, it would be nice if this could be done by some init code (released) and a module. I don't know what the overhead would be, perhaps the runtime code is so small it's not worth doing. Does that mean
Correct. I think the CONFIG option isn't worthwhile in the first place and this is why I only left the CONFIG_M686 knowing most smp kernels are compiled that way. 4096bytes of virtual vmallc space and some houndred bytes of bytecode doesn't worth the config option. If something the CONFIG_F00F would be more a documentation effort 8). But nevertheless if somebody really cares, that still make sense and it doesn't hurt. At the very least it is better than the current halfway broken CONFIG_M686. But personally I'm not going to implement it and if I would really be bothered by the halfway broken CONFIG_M686 I would drop it instead.
Quoted 4 lines
it's not worth doing the option either? It certainly would seen desirable to check for the F00F bug and if the code to handle it was not present refuse to boot right away.
Quoted 10 lines
The code actually looks so small as to be unworthy of an option, given
that many people would set it off not knowing was it was much less whether they needed it. This is not like a missing FPU where you can do a graceful reject of the instructions, if you have the bug and not the fix you are vulnerable to sudden total failures, correct?Signature · 3 lines
bill davidsen <[email protected]> CTO, TMR Associates, Inc Doing interesting things with little computers since 1979.
Andrea
Quoted 5 lines · Andrea Arcangeli
Correct. I think the CONFIG option isn't worthwhile in the first place and this is why I only left the
CONFIG_M686knowing most smp kernels are compiled that way. 4096bytes of virtual vmallc space and some houndred bytes of bytecode doesn't worth the config option. If something theCONFIG_F00Fwould be more a documentation effort 8).
nononono! CONFIG_FOOF is a derived symbol from whatever CONFIG_Mx8x is set. Much in the way we derive CONFIG_X86_WP_WORKS_OK, CONFIG_X86_PPRO_FENCE and freinds..
Signature · 2 lines
| Dave Jones. http://www.codemonkey.org.uk | SuSE Labs
Quoted 3 lines · Bill Davidsen
Since vendors (and consultants) like to build a single kernel for use on
multiple machines, it would be nice if this could be done by some init code (released) and a module.
The relevant code is (where possible) marked as __init already. So the init code gets thrown away whether needed or not.
Quoted 1 line
The code actually looks so small as to be unworthy of an option
It's not a matter of codesize, it's a correctness issue in the source. #ifndef CONFIG_M686 is wrong. It assumes a P6 is the only CPU family in existence without the bug, despite the fact there are probably close to a dozen others.
Quoted 4 lines
that many people would set it off not knowing was it was much less whether they needed it. This is not like a missing FPU where you can do a graceful reject of the instructions, if you have the bug and not the fix you are vulnerable to sudden total failures, correct?
No. You at worse vulnerable to a malicious user running hand-crafted code (no compiler generates this code-sequence) bringing down the machine.
The proposal however was not to remove anything that we currently have. Every kernel that is possible to be run on an affected box (i386/i486/i586) would still have the workaround present. We just won't generate it in Cyrix III, Athlon, Pentium 4, etc kernels..
Signature · 2 lines
| Dave Jones. http://www.codemonkey.org.uk | SuSE Labs
Quoted 6 lines · Dave Jones
On Thu, Mar 14, 2002 at 10:53:01AM -0500, Bill Davidsen wrote:
It's not a matter of codesize, it's a correctness issue in the source. #ifndef
CONFIG_M686is wrong. It assumes a P6 is the only CPU family in existence without the bug, despite the fact there are probably close to a dozen others.
No problem with that, it obviously should be done for all chips which
could have the bug, both Intel and faithful clones :-(
Quoted 7 lines
Quoted 4 lines
that many people would set it off not knowing was it was much less whether they needed it. This is not like a missing FPU where you can do a graceful reject of the instructions, if you have the bug and not the fix you are vulnerable to sudden total failures, correct?
No. You at worse vulnerable to a malicious user running hand-crafted code (no compiler generates this code-sequence) bringing down the machine.
Having the machine lock suddenly when user mode code is executed fits MY
definition of sudden total failure! Malicious user meaning anyone who d/l something and runs it. Doesn't take malicious, just dumb. Somone wanting to try the new screen saver or some such.
Quoted 4 lines
The proposal however was not to remove anything that we currently have. Every kernel that is possible to be run on an affected box (i386/i486/i586) would still have the workaround present. We just won't generate it in Cyrix III, Athlon, Pentium 4, etc kernels..
As long as it's not something the user can easily set wrong. If they
build a kernel for K6 and they have a P-II "stupidity, like virtue, is its own reward."
Signature · 3 lines
bill davidsen <[email protected]> CTO, TMR Associates, Inc Doing interesting things with little computers since 1979.
Quoted 10 lines · Dave Jones
Quoted 5 lines · Andrea Arcangeli
Correct. I think the CONFIG option isn't worthwhile in the first place and this is why I only left the
CONFIG_M686knowing most smp kernels are compiled that way. 4096bytes of virtual vmallc space and some houndred bytes of bytecode doesn't worth the config option. If something theCONFIG_F00Fwould be more a documentation effort 8).nononono!
CONFIG_FOOFis a derived symbol from whateverCONFIG_Mx8xis set. Much in the way we deriveCONFIG_X86_WP_WORKS_OK,CONFIG_X86_PPRO_FENCEand freinds..
Of course I perfectly understood what you meant. I designed the current way of doing the CONFIG_X86 some years back so I know what you mean.
Just re-read the previous email with that in mind. What I meant is that 4096bytes of virtual vmalloc space doesn't worth a CONFIG_F00F IMHO. While CONFIG_F00F isn't wasted user time because the user won't see it, it still clobbers the source code, but nevertheless it is better than the current halfway broken #ifdef CONFIG_M686 and that's why I said if somebody bothers to verymicrooptimize I'm ok, I just won't microoptimize myself and I'd drop CONFIG_M686 instead.
Andrea
Quoted 22 lines · Andrea Arcangeli
Quoted 21 lines · Bill Davidsen
Quoted 18 lines · Dave Jones
Quoted 10 lines · Andrea Arcangeli
Only in 2.4.19pre3aa2: 21_pte-highmem-f00f-1
vmalloc called before
smp_initwas an hack, right way is to use fixmap.CONFIG_M686doesn't mean much these days, but it's ok and probably most vendors will use it for the smp kernels, so it will save 4096 of the vmalloc space. I just didn't wanted to clobber the code with ||CONFIG_K7||CONFIG_... | ... given all the other f00f stuff is also conditional only to M686 and probably nobody bothered to compile it out for my same reasonBrian Gerst had a patch a few months back to introduce a
CONFIG_F00Fif a relevantCONFIG_Mxxxwas chosen[1]. It never got applied anywhere, but makes more sense than theCONFIG_M686we currently use.[1] 386/486/586. With addition of my Vendor choice menu, we could even further narrow it down to Intel only.
Quoted 8 lines
Quoted 4 lines
Since vendors (and consultants) like to build a single kernel for use on
multiple machines, it would be nice if this could be done by some init code (released) and a module. I don't know what the overhead would be, perhaps the runtime code is so small it's not worth doing. Does that meanCorrect. I think the CONFIG option isn't worthwhile in the first place and this is why I only left the
CONFIG_M686knowing most smp kernels are compiled that way. 4096bytes of virtual vmallc space and some houndred
Does that mean kernels compiled for 486 and smp won't be protected from F00F?
Quoted 12 lines · Dave Jones
Quoted 10 lines · Andrea Arcangeli
Only in 2.4.19pre3aa2: 21_pte-highmem-f00f-1
vmalloc called before
smp_initwas an hack, right way is to use fixmap.CONFIG_M686doesn't mean much these days, but it's ok and probably most vendors will use it for the smp kernels, so it will save 4096 of the vmalloc space. I just didn't wanted to clobber the code with ||CONFIG_K7||CONFIG_... | ... given all the other f00f stuff is also conditional only to M686 and probably nobody bothered to compile it out for my same reason
Quoted 3 lines
Brian Gerst had a patch a few months back to introduce a
CONFIG_F00Fif a relevantCONFIG_Mxxxwas chosen[1]. It never got applied anywhere, but makes more sense than theCONFIG_M686we currently use.
indeed.
Andrea
Quoted 4 lines · Dave Jones
[1] 386/486/586. With addition of my Vendor choice menu, we could even further narrow it down to Intel only.
Could you be convinced to post your vendor choice patch to linux-kernel perhaps? :)
(yes, I know I already have it, I would like others to see it)
Jeff
Quoted 3 lines · Jeff Garzik
Could you be convinced to post your vendor choice patch to linux-kernel perhaps? :)
(yes, I know I already have it, I would like others to see it)
Sure. it's against something ancient, and I've not had time to rediff/update it. If there's sufficient interest, I'll do so soon..
http://www.codemonkey.org.uk/patches/old/cpu-choice-2.diff
Signature · 2 lines
| Dave Jones. http://www.codemonkey.org.uk | SuSE Labs
Quoted 7 lines · Andrea Arcangeli
...
Only in 2.4.19pre3aa2: 10_vm-32Fixed ext3 deadlock and "theorical" mainline SMP race for some arch.
That works fine. ext3 is happy again.
-
Followup to: <[email protected]> By author: Dave Jones <[email protected]> In newsgroup: linux.dev.kernel
Quoted 13 lines · Andrea Arcangeli
Quoted 10 lines
Only in 2.4.19pre3aa2: 21_pte-highmem-f00f-1
vmalloc called before
smp_initwas an hack, right way is to use fixmap.CONFIG_M686doesn't mean much these days, but it's ok and probably most vendors will use it for the smp kernels, so it will save 4096 of the vmalloc space. I just didn't wanted to clobber the code with ||CONFIG_K7||CONFIG_... | ... given all the other f00f stuff is also conditional only to M686 and probably nobody bothered to compile it out for my same reason
Quoted 7 lines
Brian Gerst had a patch a few months back to introduce a
CONFIG_F00Fif a relevantCONFIG_Mxxxwas chosen[1]. It never got applied anywhere, but makes more sense than theCONFIG_M686we currently use.[1] 386/486/586. With addition of my Vendor choice menu, we could even further narrow it down to Intel only.
One thing: I would again like to see "compatibility" and "optimization" be separated out. The current CPU type menu is a bit of both.
It's actually quite useful these days to compile for 386 or 486, but optimize for 686.
-hpa
Signature · 3 lines
<[email protected]> at work, <[email protected]> in private! "Unix gives you enough rope to shoot yourself in the foot." http://www.zytor.com/~hpa/puzzle.txt <[email protected]>
Quoted 12 lines · Andrea Arcangeli
Only in 2.4.19pre3aa2: 00_loop-IV-API-hvr-1
Make the IV API not to be in function of the blkdev of the underlying fs, so you can copy your cryptoloop around without risking to lose data. This breaks the on-disk format of some encrypted transfer module though, if you don't like it please discuss it here in CC with Herbert Valerio Riedel <[email protected]>, the patch
is from him. I think writing a converter in place of the loop data would be the preferred solution if needed. It could be done in a way to link transparently with the source of the kernel modules.
Since you are finally fixing loop bugs, attached is a patch of loop fixes from loop-AES package (cipher code has been removed of course). These fixes are well tested and are basically unchanged since September 29 2001. Biggest fix is to make device backed loop work with swap so that encrypted swap is possible:
- IV computed in 512 byte units.
- Make device backed loop work with swap by pre-allocating pages.
- External encryption module locking bug fixed (from Ingo Rohloff).
- Get rid of the loop_get_bs() crap.
- grab_cache_page() return value handled properly, avoids Oops.
- No more illegal messing with BH_Dirty flag.
- No more illegal sleeping in generic_make_request().
- Loops can be set-up properly when root partition is still mounted ro.
- Default soft block size is set properly for file backed loops.
- kmalloc() error case handled properly.
- loop_set_fd() 'goto out_putf' error case handled properly.
Regards,
Jari Ruusu <[email protected]>
hello!
Quoted 13 lines · Jari Ruusu
Quoted 12 lines · Andrea Arcangeli
Only in 2.4.19pre3aa2: 00_loop-IV-API-hvr-1
Make the IV API not to be in function of the blkdev of the underlying fs, so you can copy your cryptoloop around without risking to lose data. This breaks the on-disk format of some encrypted transfer module though, if you don't like it please discuss it here in CC with Herbert Valerio Riedel <[email protected]>, the patch
is from him. I think writing a converter in place of the loop data would be the preferred solution if needed. It could be done in a way to link transparently with the source of the kernel modules.
just as a side note to compatibility to the old IV metric;
the patch I sent to andrea is quite minimal, it only changes the IV metric, and adds a few #define's to loop.h in order to recognize the IV metric when compiling filter modules against it; as to jari's patch, if the following def's were added, it can be used instead of my minimal patch...
/* definitions for IV metric */
#define LOOP_IV_SECTOR_BITS 9
#define LOOP_IV_SECTOR_SIZE (1 << LOOP_IV_SECTOR_BITS)
typedef int loop_iv_t;
...except maybe for when backward compatibility is needed. As it is a major concern to some of us to be able to convert their old iv-metric encrypted volumes to the new "atomic"-IV-metric, it can be usefull to be able to have both IV metrics available on the same system...
one approach could be to losetup the concerned volume w/ an old loop.o v ersion and decipher the content on-the-fly by doing something like
# set up the loop dev losetup -e cipher /dev/loop0 /dev/partition
# make sure we got the right passphrase etc...; and also make sure the # kernel fs code set the right transfer-block-size... mount /dev/loop0 /mnt/tmp; umount /dev/loop0
# decrypt on-the-fly... dd if=/dev/loop0 of=/dev/partition
...now in theory we should have the unencrypted data lying in /dev/partition
now we could replace the loop.o module by one feature the new IV metric, and just reverse the process..
so far ok... but some of us might want to decrypt and encrypt directly, w/o having to have some unencrypted (thus unprotected) data lying around for the meantime; so we might want to have 2 IV metrics available at the same time... this can either be done by cooperation from the loop.o module, i.e. some option for setting the IV size to calculate... _or_ it can be calculated by the filter module, by finding out what IV blocksize to assume, and then dividing the 512-IV-metric by some integral factor to reflect the old metric...
the latter approach is possible w/ my patch, and has been implemented cryptoapi's cryptoloop filter through ioctl()...
this relays on the IV-fix not modifying the transfer block size (if the blocks passed to the transfer filter should become smaller than the old metric's IV block size, we simply can't emulate the old metric...), but only the passed IV value...
ps: just to make one thing clear (again), I don't care too much whether my loop-fix or jari's goes in; I primarily care for a fixed IV situation (including the above mentioned #define's/typedef) and if possible anyhow to allow for limited compatibility to the old metric...
regards,
Signature · 4 lines
Herbert Valerio Riedel / Phone: (EUROPE) +43-1-58801-18840 Email: [email protected] / Finger [email protected] for GnuPG Public Key GnuPG Key Fingerprint: 7BB9 2D6C D485 CE64 4748 5F65 4981 E064 883F 4142
Quoted 1 line · Jari Ruusu
- No more illegal sleeping in
generic_make_request().
I've told you this before -- sleeping in make_request is not illegal, heck it happens _all the time_. Safely sleeping requires a reserved pool of the units you wish to allocate, of course. In fact I think that would be much nicer than the path you are following here by delaying allocations to the loop thread (and still not using a reserved pool).
Briefly looking through the patch.
- loop_put_buffer(), it looks racy to check waitqueue_active there.
- if(!bh) return((struct buffer_head *)0);
eww!
- Also, please adher to the style. VaRiAbLe names can hurt the eyes, and stuff like
if (something) break; return(val);
etc don't belong too. Could you fix that up?
That said, thanks for fixing it!
Signature · 1 line
Jens Axboe
Quoted 2 lines · Jens Axboe
Quoted 1 line · Jari Ruusu
- No more illegal sleeping in
generic_make_request().
BTW, it looks like you are killing LO_FLAGS_BH_REMAP?! Why? This is a very worthwhile optimization.
Signature · 1 line
Jens Axboe
Quoted 8 lines · Jens Axboe
Quoted 1 line · Jari Ruusu
- No more illegal sleeping in
generic_make_request().I've told you this before -- sleeping in
make_requestis not illegal, heck it happens_allthetime_. Safely sleeping requires a reserved pool of the units you wish to allocate, of course. In fact I think that would be much nicer than the path you are following here by delaying allocations to the loop thread (and still not using a reserved pool).
Yes, I know you have told me that before, but I'm being overcareful. See:
<quote> from device drivers book by Alessandro Rubini, chapter 12, page 331 The request function has one very important constraint: it must be atomic. request is not usually called in direct response to user requests, and it is not running in the context of any particular process. It can be called at interrupt time, from tasklets, or from any number of other places. Thus, it must not sleep while carrying out its tasks.
</quote>
Quoted 1 line
-
loop_put_buffer(), it looks racy to checkwaitqueue_activethere.
No race there. All that loop_put_buffer() cares is that helper thread wakes up. If helper thread woke up earlier and completed its job, fine. If helper thread wakes up later, that is fine too. If helper thread wakes up unnecessarily, it will just go back to sleep after noticing that that there is no work to do.
Quoted 7 lines
- if(!bh)
return((struct buffer_head *)0);eww!
- Also, please adher to the style. VaRiAbLe names can hurt the eyes, and stuff like
Quoted 7 lines
if (something) break; return(val);etc don't belong too. Could you fix that up?
That said, thanks for fixing it!
If there is any chance of being merged to mainline kernel, I will fix these "hurt the eyes" formatting issues.
Quoted 2 lines
BTW, it looks like you are killing
LO_FLAGS_BH_REMAP?! Why? This is a very worthwhile optimization.
Removing it simplified the code a lot. Doing remap direcly from loop_make_request() would probably be more effective. Just remap and return 1 from loop_make_request() like LVM code does.
Regards,
Jari Ruusu <[email protected]>
Quoted 10 lines · Herbert Valerio Riedel
the patch I sent to andrea is quite minimal, it only changes the IV metric, and adds a few #define's to
loop.hin order to recognize the IV metric when compiling filter modules against it; as to jari's patch, if the following def's were added, it can be used instead of my minimal patch.../* definitions for IV metric */
#defineLOOP_IV_SECTOR_BITS9
#defineLOOP_IV_SECTOR_SIZE(1 <<LOOP_IV_SECTOR_BITS)
Quoted 6 lines
typedef int
loop_iv_t;...except maybe for when backward compatibility is needed. As it is a major concern to some of us to be able to convert their old iv-metric encrypted volumes to the new "atomic"-IV-metric, it can be usefull to be able to have both IV metrics available on the same system...
Not changing IV parameter type in 2.4 kernels is important. Break that in 2.5/2.6 kernels, but not in stable 2.4, ok? Older 2.4 kernels dont't have loop_iv_t, and being able to compile _existing_ modules for them is important. 512 byte IV is nothing new, you know that, and all sane systems have used 512 byte IV for a long time already. So the 'block size IV' change to '512 byte IV' is nothing new, but changing the parameter type is evil and should be avoided for compatibility sake.
Quoted 4 lines
ps: just to make one thing clear (again), I don't care too much whether my loop-fix or jari's goes in; I primarily care for a fixed IV situation (including the above mentioned #define's/typedef) and if possible anyhow to allow for limited compatibility to the old metric...
So the choice here is either break (or at least cause need to modify) all other implementations or cryptoapi implementation.
Herbert, if this loop_iv_t type goes into mainline kernel, I will have to reverse that on loop-AES patches for backward compatibility.
Dependency on above mentioned #define's/typedef on kernel include files is silly, as cryptoapi can define them on any of its own include files.
Regards,
Jari Ruusu <[email protected]>
hello!
Quoted 8 lines · Jari Ruusu
Quoted 5 lines
/* definitions for IV metric */
#defineLOOP_IV_SECTOR_BITS9
#defineLOOP_IV_SECTOR_SIZE(1 <<LOOP_IV_SECTOR_BITS)typedef int
loop_iv_t;Not changing IV parameter type in 2.4 kernels is important. Break that in 2.5/2.6 kernels, but not in stable 2.4, ok?
if you look closely at the typedef above, you'll notice that it doesn't introduce a _new_ type, but only a synonym for the 'int' type, which happens to be the type used in 2.4 kernels...
typedef int (* transfer_proc_t)(struct loop_device *, int cmd,
char *raw_buf, char *loop_buf, int size, int real_block);
what I'm trying to do by the introduction of this _aesthetic_ syntactic element, is to prepare the way to smoothly upgrade the type used for the IV value; and more important to keep filter modules from having to add ugly #ifdef LINUX_VERSION_CODE <= KERNEL(2,X,Y)... just for changes which could be catched by kernel headers...
have you ever wondered, why even the C standard defines some typedefs as size_t instead of using just a 'long'?
Quoted 3 lines
Older 2.4 kernels dont't have
loop_iv_t, and being able to compile _existing_ modules for them is important.
as older kernels will likely have the variable-IV-metric, they'll have to be patched anyway (loop.[ch]), and after having patched them, the problem vanishes...
Quoted 6 lines
Quoted 4 lines
ps: just to make one thing clear (again), I don't care too much whether my loop-fix or jari's goes in; I primarily care for a fixed IV situation (including the above mentioned #define's/typedef) and if possible anyhow to allow for limited compatibility to the old metric...
So the choice here is either break (or at least cause need to modify) all other implementations or cryptoapi implementation.
actually it's just -- either make my life harder w/ cryptoapi or not, as it doesn't affect other implementations; but other implementations may start to use those typedefs in the future, so when 2.6 (or whatever comes out) the 'current' 2.6 has them already, and the 'ultra-stable' 2.4's will have them as well... and maybe everybody is happy, that he can compile the same c source against both, 2.4 and 2.6, loop.h and still get a correct filter module...
Quoted 2 lines
Herbert, if this
loop_iv_ttype goes into mainline kernel, I will have to reverse that on loop-AES patches for backward compatibility.
I don't see why... loop_iv_t is just a 'reviced-declarator-type-list int', i.e. it is is type compatible w/ an 'int'... so you won't even notice the difference when compiling loop-AES against it...
Quoted 2 lines
Dependency on above mentioned #define's/typedef on kernel include files is silly, as cryptoapi can define them on any of its own include files.
yeah, sure... at the expense of having to redundantly keep track (ugly #ifdef's) of which kernel version is being used...
another reason I haven't mentioned so far is about the #define's;
#define LOOP_IV_SECTOR_BITS 9
#define LOOP_IV_SECTOR_SIZE (1 << LOOP_IV_SECTOR_BITS)
...if you don't see any reason for those 2 innocent #define then you won't see any reason for other #define in the kernel headers as well; e.g.:
fs.h:
#define BLOCK_SIZE_BITS 10
#define BLOCK_SIZE (1<<BLOCK_SIZE_BITS)
you may ask why those are used, since they are hardcoded anyway... well, lemme show you an example where this would considerably help:
loop.c:
static unsigned long compute_loop_size(struct loop_device *lo, struct dentry * lo_dentry, kdev_t lodev)
{
if (S_ISREG(lo_dentry->d_inode->i_mode))
return (lo_dentry->d_inode->i_size - lo->lo_offset) >> BLOCK_SIZE_BITS;
if (blk_size[MAJOR(lodev)])
return blk_size[MAJOR(lodev)][MINOR(lodev)] -
(lo->lo_offset >> BLOCK_SIZE_BITS);
return MAX_DISK_SIZE;
}and now imagine the same source fragment, with the #define's evaluated... wouldn't that be less self-explaining?
well, I hope I could make clear to you, why using symbolic constants and the use of typedefs in C are good practice in general wrt software engineering...
regards,
Signature · 4 lines
Herbert Valerio Riedel / Phone: (EUROPE) +43-1-58801-18840 Email: [email protected] / Finger [email protected] for GnuPG Public Key GnuPG Key Fingerprint: 7BB9 2D6C D485 CE64 4748 5F65 4981 E064 883F 4142
Quoted 12 lines · Jari Ruusu
Quoted 8 lines · Jens Axboe
Quoted 1 line · Jari Ruusu
- No more illegal sleeping in
generic_make_request().I've told you this before -- sleeping in
make_requestis not illegal, heck it happens_allthetime_. Safely sleeping requires a reserved pool of the units you wish to allocate, of course. In fact I think that would be much nicer than the path you are following here by delaying allocations to the loop thread (and still not using a reserved pool).Yes, I know you have told me that before, but I'm being overcareful. See:
Quoted 7 lines
<quote> from device drivers book by Alessandro Rubini, chapter 12, page 331 The request function has one very important constraint: it must be atomic. request is not usually called in direct response to user requests, and it is not running in the context of any particular process. It can be called at interrupt time, from tasklets, or from any number of other places. Thus, it must not sleep while carrying out its tasks.
</quote>
loop isn't implement via ->request_fn anymore. Loop since 2.4 is only driven by the ->make_request_fn, that for the other more normal devices just means the old legacy __make_request. request_fn is subject to the rules pointed out by Alessandro, but ->make_request_fn can sleep just like ll_rw_block can sleep. ->make_request_fn and in turn loop_make_request can only run in normal context with irq enabled and they're both allowed to sleep just like ll_rw_block and submit_bh.
Nevertheless as Jens said the infinite-loop-allocation in the ->make_request_fn path are deadlock prone at the moment, not because they sleeps but because they need a reserved mempool to guarantee operations can go ahead slowly without deadlocks even if dynamic allocation fails, but this is not a very pratical problem, it's very unlikely to deadlock there (it's not worse than the other infinite loop in getblk() that affects not just loop).
Andrea
Quoted 7 lines · Andrea Arcangeli
Nevertheless as Jens said the infinite-loop-allocation in the ->make_request_fn path are deadlock prone at the moment, not because they sleeps but because they need a reserved mempool to guarantee operations can go ahead slowly without deadlocks even if dynamic allocation fails, but this is not a very pratical problem, it's very unlikely to deadlock there (it's not worse than the other infinite loop in
getblk()that affects not just loop).
Unlikely to deadlock for normal filesystems, but swap on encrypted loop is a different case.
Deadlock free operation is exactly why my prealloc loop patch is needed. Beyond initial preallocating that is done at losetup time, it does not allocate anything from kernel memory pools, but effectively recycles its private per loop device preallocated memory. Encrypted swap needs that, and normal device backed loop file systems are also happy with deadlock free and VM stress free operation.
Regards,
Jari Ruusu <[email protected]>
Quoted 7 lines · Herbert Valerio Riedel
Quoted 2 lines · Jari Ruusu
Not changing IV parameter type in 2.4 kernels is important. Break that in 2.5/2.6 kernels, but not in stable 2.4, ok?
if you look closely at the typedef above, you'll notice that it doesn't introduce a _new_ type, but only a synonym for the 'int' type, which happens to be the type used in 2.4 kernels...
A synonym that does not exist on older kernels.
Quoted 3 lines
typedef int (* transfer_proc_t)(struct
loop_device*, int cmd,
char*raw_buf, char*loop_buf, int size, intreal_block);
^^^
That function prototype _must_ _not_ change in 2.4 kernels. That 'int' better be there on newer 2.4 kernels.
Quoted 6 lines
Quoted 3 lines
Older 2.4 kernels dont't have
loop_iv_t, and being able to compile _existing_ modules for them is important.as older kernels will likely have the variable-IV-metric, they'll have to be patched anyway (loop.[ch]), and after having patched them, the problem vanishes...
If you haven't noticed, loop-AES compiles and works fine on older kernels without changing anything in kernel sources.
Quoted 4 lines
Quoted 2 lines
So the choice here is either break (or at least cause need to modify) all other implementations or cryptoapi implementation.
actually it's just -- either make my life harder w/ cryptoapi or not, as it doesn't affect other implementations;
Transfer function prototype change does affect other implementations. I would have to add workaround to compile loop-AES on older kernels. If cryptoapi got one parameter wrong, I don't see why other implementations should bend over because of that.
Herbert, if you need some special types and/or macros in cryptoapi, just define them in cryptoapi include files. Please don't mess with code that other implementations depend on.
Quoted 5 lines
Quoted 2 lines
Herbert, if this
loop_iv_ttype goes into mainline kernel, I will have to reverse that on loop-AES patches for backward compatibility.I don't see why...
loop_iv_tis just a 'reviced-declarator-type-list int', i.e. it is is type compatible w/ an 'int'... so you won't even notice the difference when compiling loop-AES against it...
What I do notice is having to add workaround. See, loop-AES only creates patched-loop.c (but not patched-loop.h), and under some circumstances patched-loop.c may come from newer code than loop.h. So it needs a workaround if transfer function prototype is changed.
Quoted 4 lines
Quoted 2 lines
Dependency on above mentioned #define's/typedef on kernel include files is silly, as cryptoapi can define them on any of its own include files.
yeah, sure... at the expense of having to redundantly keep track (ugly #ifdef's) of which kernel version is being used...
If IV parameter type is changed from 32 bits to 64 bits, you will need new code and #ifdef's to handle that anyway. And that is 2.5/2.6 thing anyway.
Regards,
Jari Ruusu <[email protected]>
Quoted 11 lines · Jari Ruusu
Quoted 7 lines · Andrea Arcangeli
Nevertheless as Jens said the infinite-loop-allocation in the ->make_request_fn path are deadlock prone at the moment, not because they sleeps but because they need a reserved mempool to guarantee operations can go ahead slowly without deadlocks even if dynamic allocation fails, but this is not a very pratical problem, it's very unlikely to deadlock there (it's not worse than the other infinite loop in
getblk()that affects not just loop).Unlikely to deadlock for normal filesystems, but swap on encrypted loop is a different case.
Not really much different, it's no different than other paths like getblk (when you swap on top of the fs) and brw_page (OTOH we have a little reserved shared pool for the async bh needed by brw_page but it's not math accurate first of all because it's shared for the other pagecache I/O too), it just increases a little the mem pressure when there's a transfer function involved and the translation cannot be done in place, but I obviously agree that such loop deadlock needs fixing reagardless if it's easy to reproduce it or not :). Infact I think I mentioned such deadlock in the past too.
Quoted 6 lines
Deadlock free operation is exactly why my prealloc loop patch is needed. Beyond initial preallocating that is done at losetup time, it does not allocate anything from kernel memory pools, but effectively recycles its private per loop device preallocated memory. Encrypted swap needs that, and normal device backed loop file systems are also happy with deadlock free and VM stress free operation.
Yes, thanks.
Andrea
Quoted 12 lines · Jari Ruusu
Quoted 8 lines · Jens Axboe
Quoted 1 line · Jari Ruusu
- No more illegal sleeping in
generic_make_request().I've told you this before -- sleeping in
make_requestis not illegal, heck it happens_allthetime_. Safely sleeping requires a reserved pool of the units you wish to allocate, of course. In fact I think that would be much nicer than the path you are following here by delaying allocations to the loop thread (and still not using a reserved pool).Yes, I know you have told me that before, but I'm being overcareful. See:
Quoted 7 lines
<quote> from device drivers book by Alessandro Rubini, chapter 12, page 331 The request function has one very important constraint: it must be atomic. request is not usually called in direct response to user requests, and it is not running in the context of any particular process. It can be called at interrupt time, from tasklets, or from any number of other places. Thus, it must not sleep while carrying out its tasks.
</quote>
That's talking about the request_fn funtion, not related to make_request_fn that I rewrote loop to use. So that's not a valid point.
Quoted 9 lines
Quoted 9 lines
- if(!bh)
return((struct buffer_head *)0);eww!
- Also, please adher to the style. VaRiAbLe names can hurt the eyes, and stuff like
if (something) break;
Quoted 8 lines
Quoted 5 lines
return(val);etc don't belong too. Could you fix that up?
That said, thanks for fixing it!
If there is any chance of being merged to mainline kernel, I will fix these "hurt the eyes" formatting issues.
I think there is. At least I can safely say there's no chance it will be merged if these things aren't fixed. So take your pick :-)
Quoted 5 lines
Quoted 2 lines
BTW, it looks like you are killing
LO_FLAGS_BH_REMAP?! Why? This is a very worthwhile optimization.Removing it simplified the code a lot. Doing remap direcly from
loop_make_request()would probably be more effective. Just remap and return
LOTS more effective. Please don't kill this functionality. I don't buy the simplification argument.
Quoted 1 line
1 from
loop_make_request()like LVM code does.
And like loop currently does...
Signature · 1 line
Jens Axboe
Quoted 6 lines · Jens Axboe
Quoted 2 lines · Jari Ruusu
If there is any chance of being merged to mainline kernel, I will fix these "hurt the eyes" formatting issues.
I think there is. At least I can safely say there's no chance it will be merged if these things aren't fixed. So take your pick :-)
OK, I have fixed above mentioned formatting issues. A patch is attached.
And, about sleeping in loop_make_request(), I have also changed the code in such way that it defaults to code that may sleep in loop_make_request(). But non-sleeping code is still present (but not currently used), like this:
#if 1
may-sleep-in-loop_make_request-code-here
#else
non-sleeping-loop_make_request-code-here
#endif
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
BTW, it looks like you are killing
LO_FLAGS_BH_REMAP?! Why? This is a very worthwhile optimization.Removing it simplified the code a lot. Doing remap direcly from
loop_make_request()would probably be more effective. Just remap and returnLOTS more effective. Please don't kill this functionality. I don't buy the simplification argument.
This new patch does not remove LO_FLAGS_BH_REMAP optimization.
Quoted 3 lines
Quoted 1 line
1 from
loop_make_request()like LVM code does.And like loop currently does...
2.4.19-pre3 loop code wanders to loop_get_buffer() and transfer function in LO_FLAGS_BH_REMAP optimization case.
My version is simpler than yours:
if (lo->lo_flags & LO_FLAGS_BH_REMAP) {
rbh->b_rsector += (lo->lo_offset >> 9);
rbh->b_rdev = lo->lo_device;
return 1;
} Regards,
Jari Ruusu <[email protected]>
Quoted 5 lines · Jari Ruusu
Quoted 4 lines
Quoted 2 lines
If there is any chance of being merged to mainline kernel, I will fix these "hurt the eyes" formatting issues.
I think there is. At least I can safely say there's no chance it will be merged if these things aren't fixed. So take your pick :-)
OK, I have fixed above mentioned formatting issues. A patch is attached.
...well, you still haven't honored my modest wish of the innocent 2 #define's and 1 typedef...
regards,
Signature · 4 lines
Herbert Valerio Riedel / Phone: (EUROPE) +43-1-58801-18840 Email: [email protected] / Finger [email protected] for GnuPG Public Key GnuPG Key Fingerprint: 7BB9 2D6C D485 CE64 4748 5F65 4981 E064 883F 4142
Quoted 9 lines · Jari Ruusu
Quoted 6 lines · Jens Axboe
Quoted 2 lines · Jari Ruusu
If there is any chance of being merged to mainline kernel, I will fix these "hurt the eyes" formatting issues.
I think there is. At least I can safely say there's no chance it will be merged if these things aren't fixed. So take your pick :-)
OK, I have fixed above mentioned formatting issues. A patch is attached.
Super!
Quoted 9 lines
And, about sleeping in
loop_make_request(), I have also changed the code in such way that it defaults to code that may sleep inloop_make_request(). But non-sleeping code is still present (but not currently used), like this:#if 1
may-sleep-in-loop_make_request-code-here
#else
non-sleeping-loop_make_request-code-here
#endif
The solution I prefer is to have loop_make_request() block when we run out of pre-allocated buffers, ie similar to "normal" block drivers when they run out of request slots. This provides a nice throttling mechanism and makes sure that loop doesn't eat into the memory polls too heavily.
In any way, do it one way and remove the other.
Quoted 10 lines
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
BTW, it looks like you are killing
LO_FLAGS_BH_REMAP?! Why? This is a very worthwhile optimization.Removing it simplified the code a lot. Doing remap direcly from
loop_make_request()would probably be more effective. Just remap and returnLOTS more effective. Please don't kill this functionality. I don't buy the simplification argument.
This new patch does not remove
LO_FLAGS_BH_REMAPoptimization.
Good.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
1 from
loop_make_request()like LVM code does.And like loop currently does...
2.4.19-pre3 loop code wanders to
loop_get_buffer()and transfer function inLO_FLAGS_BH_REMAPoptimization case.
Quoted 7 lines
My version is simpler than yours:
if (lo->lo_flags & LO_FLAGS_BH_REMAP) { rbh->b_rsector += (lo->lo_offset >> 9); rbh->b_rdev = lo->lo_device; return 1; }
So the 'new' version does exactly the same, but doesn't do it in loop_get_buffer().
Signature · 1 line
Jens Axboe
Quoted 11 lines · Jens Axboe
Quoted 9 lines · Jari Ruusu
And, about sleeping in
loop_make_request(), I have also changed the code in such way that it defaults to code that may sleep inloop_make_request(). But non-sleeping code is still present (but not currently used), like this:#if 1
may-sleep-in-loop_make_request-code-here
#else
non-sleeping-loop_make_request-code-here
#endif
Quoted 6 lines
The solution I prefer is to have
loop_make_request()block when we run out of pre-allocated buffers, ie similar to "normal" block drivers when they run out of request slots. This provides a nice throttling mechanism and makes sure that loop doesn't eat into the memory polls too heavily.In any way, do it one way and remove the other.
OK, non-sleeping code is gone.
Quoted 5 lines
Quoted 2 lines
2.4.19-pre3 loop code wanders to
loop_get_buffer()and transfer function inLO_FLAGS_BH_REMAPoptimization case.So the 'new' version does exactly the same, but doesn't do it in
loop_get_buffer().
I found a new bug in stock loop driver: LO_FLAGS_BH_REMAP optimization is never enabled because init hook for type 0 transfer is never executed. That bug is not dangerous, but is not fixed by patch that I posted here yesterday... so here is new one that fixes it.
Quoted 2 lines · Herbert Valerio Riedel
...well, you still haven't honored my modest wish of the innocent 2 #define's and 1 typedef...
Your cryptoapi specific defines and typedef are now included.
Regards,
Jari Ruusu <[email protected]>