Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
After all the preparatory changes are committed, this is a final[*] notice/warning that I am going to start committing the following patchset really soon now[**[: http://people.freebsd.org/~avg/zfsboot.patches.9.diff [*] unless circumstances change [**] maybe next hour, even -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 08/05/2012 17:15 John Baldwin said the following: > Bruce might even suggest adding a ba_ prefix to all the members of > struct bootargs btw. I would not be opposed, but you've already done > a fair bit of work on this patch. Thank you for sparing me :-) So I hope to get busy committing this stuff soon. -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On 5/8/12 3:14 AM, Andriy Gapon wrote: > on 07/05/2012 20:43 John Baldwin said the following: >> On Monday, May 07, 2012 10:47:05 am Andriy Gapon wrote: >>> on 07/05/2012 16:53 John Baldwin said the following: > [snip] >>> What do you think about the -LOCORE- change that Bruce inspired? >> >> In general I think this looks good. I have only one suggestion. In other >> code (e.g. the genassym constants in the kernel) where we define constants >> for field offsets, we make the constant be the uppercase name of the field >> itself (e.g. TD_PCB for offsetof(struct thread, td_pcb)). I would rather >> do that here as well. In this case the field names do not have a prefix, >> but let's just use a BA_ prefix for members of 'bootargs'. BI_SIZE is >> already correct, but this would mean renaming HT_OFF to BA_HOWTO, BF_OFF to >> BA_BOOTFLAGS, and BI_OFF to BA_BOOTINFO. > > OK, doing this. > >> I think you can probably leave BA_SIZE as-is. > > I see that i386 genassym has a few different styles for sizeof constants: > ABBRSIZE > FULL_NAME_SIZE > ABBR_SIZEOF > > FULL_NAME_SIZE looked the most appealing to me (and seems to be the most > common), so I decided to change BA_SIZE to BOOTARGS_SIZE. > I hope that this makes sense and I am not starting a bikeshed :-) Yeah, given the inconsistency in sizeof() constants in genassym.c, just about anything is fine, which is why I hesitated to suggest any change. BOOTARGS_SIZE is fine. I probably slightly prefer that because it is less ambiguous (in case the structure has a foo_size member such as bi_size in bootinfo). Bruce might even suggest adding a ba_ prefix to all the members of struct bootargs btw. I would not be opposed, but you've already done a fair bit of work on this patch. -- John Baldwin ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 07/05/2012 20:43 John Baldwin said the following: > On Monday, May 07, 2012 10:47:05 am Andriy Gapon wrote: >> on 07/05/2012 16:53 John Baldwin said the following: [snip] >> What do you think about the -LOCORE- change that Bruce inspired? > > In general I think this looks good. I have only one suggestion. In other > code (e.g. the genassym constants in the kernel) where we define constants > for field offsets, we make the constant be the uppercase name of the field > itself (e.g. TD_PCB for offsetof(struct thread, td_pcb)). I would rather > do that here as well. In this case the field names do not have a prefix, > but let's just use a BA_ prefix for members of 'bootargs'. BI_SIZE is > already correct, but this would mean renaming HT_OFF to BA_HOWTO, BF_OFF to > BA_BOOTFLAGS, and BI_OFF to BA_BOOTINFO. OK, doing this. > I think you can probably leave BA_SIZE as-is. I see that i386 genassym has a few different styles for sizeof constants: ABBRSIZE FULL_NAME_SIZE ABBR_SIZEOF FULL_NAME_SIZE looked the most appealing to me (and seems to be the most common), so I decided to change BA_SIZE to BOOTARGS_SIZE. I hope that this makes sense and I am not starting a bikeshed :-) >>> Also, at some point we could use a genassym.c file ala the kernel builds to >>> generate >>> some of the constants in bootargs.h instead (e.g. the offsets of fields >>> within >>> structures, and BA_SIZE, though we probably want to ensure that BA_SIZE >>> never >>> changes). >> >> The genassym approach sounds good, but, indeed - later :) > > Yes, that can wait. I think it would not be very hard to do however. All > you really need is access to sys/kern/genassym.sh and nm. > -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Monday, May 07, 2012 11:15:53 am Andriy Gapon wrote: > on 07/05/2012 17:47 Andriy Gapon said the following: > > on 07/05/2012 16:53 John Baldwin said the following: > >> Ok. Maybe add one comment to the bootargs.h head to explain that the > >> 'bootargs' > >> struct starts at ARGOFF and can grow up, while struct bootinfo is copied > >> such that > >> it's end is at the top of the argument area and grows down. > > > > Will do. > > Could you please check the wording and correct it or suggest alternatives? > Thank you. > > diff --git a/sys/boot/i386/common/bootargs.h b/sys/boot/i386/common/bootargs.h > index 510efdd..8bc1b32 100644 > --- a/sys/boot/i386/common/bootargs.h > +++ b/sys/boot/i386/common/bootargs.h > @@ -29,6 +29,15 @@ > #define BF_OFF 8 /* offsetof(struct bootargs, bootflags) > */ > #define BI_OFF 20 /* offsetof(struct bootargs, bootinfo) > */ > > +/* > + * We reserve some space above BTX allocated stack for the arguments > + * and certain data that could hang off them. Currently only struct bootinfo > + * is supported in that category. The bootinfo is placed at the top > + * of the arguments area and the actual arguments are placed at ARGOFF offset > + * from the top and grow towards the top. Hopefully we have enough space > + * for bootinfo and the arguments to not run into each other. > + * Arguments area below ARGOFF is reserved for future use. > + */ > #define ARGSPACE0x1000 /* total size of the BTX args area */ > #define ARGOFF 0x800 /* actual args offset within the args > area */ > #define ARGADJ (ARGSPACE - ARGOFF) I think this is good, thanks! -- John Baldwin ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Monday, May 07, 2012 10:47:05 am Andriy Gapon wrote: > on 07/05/2012 16:53 John Baldwin said the following: > > On Saturday, May 05, 2012 5:53:07 am Andriy Gapon wrote: > [snip] > >> The new patchset: http://people.freebsd.org/~avg/zfsboot.patches.7.diff > > > > Looks great, thanks! A few replies below: > > Here's a followup patch for the suggestions: > http://people.freebsd.org/~avg/bootargs.followup.diff > I will merge it into the main patch. > > What do you think about the -LOCORE- change that Bruce inspired? In general I think this looks good. I have only one suggestion. In other code (e.g. the genassym constants in the kernel) where we define constants for field offsets, we make the constant be the uppercase name of the field itself (e.g. TD_PCB for offsetof(struct thread, td_pcb)). I would rather do that here as well. In this case the field names do not have a prefix, but let's just use a BA_ prefix for members of 'bootargs'. BI_SIZE is already correct, but this would mean renaming HT_OFF to BA_HOWTO, BF_OFF to BA_BOOTFLAGS, and BI_OFF to BA_BOOTINFO. I think you can probably leave BA_SIZE as-is. > > Also, at some point we could use a genassym.c file ala the kernel builds to > > generate > > some of the constants in bootargs.h instead (e.g. the offsets of fields > > within > > structures, and BA_SIZE, though we probably want to ensure that BA_SIZE > > never > > changes). > > The genassym approach sounds good, but, indeed - later :) Yes, that can wait. I think it would not be very hard to do however. All you really need is access to sys/kern/genassym.sh and nm. -- John Baldwin ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 07/05/2012 17:47 Andriy Gapon said the following: > on 07/05/2012 16:53 John Baldwin said the following: >> Ok. Maybe add one comment to the bootargs.h head to explain that the >> 'bootargs' >> struct starts at ARGOFF and can grow up, while struct bootinfo is copied >> such that >> it's end is at the top of the argument area and grows down. > > Will do. Could you please check the wording and correct it or suggest alternatives? Thank you. diff --git a/sys/boot/i386/common/bootargs.h b/sys/boot/i386/common/bootargs.h index 510efdd..8bc1b32 100644 --- a/sys/boot/i386/common/bootargs.h +++ b/sys/boot/i386/common/bootargs.h @@ -29,6 +29,15 @@ #defineBF_OFF 8 /* offsetof(struct bootargs, bootflags) */ #defineBI_OFF 20 /* offsetof(struct bootargs, bootinfo) */ +/* + * We reserve some space above BTX allocated stack for the arguments + * and certain data that could hang off them. Currently only struct bootinfo + * is supported in that category. The bootinfo is placed at the top + * of the arguments area and the actual arguments are placed at ARGOFF offset + * from the top and grow towards the top. Hopefully we have enough space + * for bootinfo and the arguments to not run into each other. + * Arguments area below ARGOFF is reserved for future use. + */ #defineARGSPACE0x1000 /* total size of the BTX args area */ #defineARGOFF 0x800 /* actual args offset within the args area */ #defineARGADJ (ARGSPACE - ARGOFF) -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 07/05/2012 16:53 John Baldwin said the following: > On Saturday, May 05, 2012 5:53:07 am Andriy Gapon wrote: [snip] >> The new patchset: http://people.freebsd.org/~avg/zfsboot.patches.7.diff > > Looks great, thanks! A few replies below: Here's a followup patch for the suggestions: http://people.freebsd.org/~avg/bootargs.followup.diff I will merge it into the main patch. What do you think about the -LOCORE- change that Bruce inspired? - Add a CTASSERT() in loader/main.c that BI_SIZE == sizeof(struct bootinfo) >>> >>> I have added a definition of CTASSERT to boostrap.h as it was not available >>> for >>> sys/boot and there were two local definitions of the macro in individual >>> files. >>> >>> However the assertion would fail right now. >>> The backward-compatible value of BI_SIZE (72 == 0x48) covers only part of >>> the >>> fields in struct bootinfo, those up to the following comment: >>> /* Items below only from advanced bootloader */ >>> >>> I am a little bit hesitant: should I increase BI_SIZE to cover the whole >>> struct >>> bootinfo or should I compare BI_SIZE to offsetof bi_kernend? > > Actually, we should probably be reading the 'bi_size' field and not using a > BI_SIZE > constant at all? Done in the above patch. > Looks like only the non-functional EFI boot loader doesn't set bi_size (and > it should > just be fixed to do so since it needs to pass new fields in anyway). > >>> I've decided to define ARGADJ in the new common header, then I've had to >>> rename >>> btxcsu.s to .S, so that the preprocessing is executed for it. > > Ok. Maybe add one comment to the bootargs.h head to explain that the > 'bootargs' > struct starts at ARGOFF and can grow up, while struct bootinfo is copied such > that > it's end is at the top of the argument area and grows down. Will do. > Also, at some point we could use a genassym.c file ala the kernel builds to > generate > some of the constants in bootargs.h instead (e.g. the offsets of fields within > structures, and BA_SIZE, though we probably want to ensure that BA_SIZE never > changes). The genassym approach sounds good, but, indeed - later :) Thank you. -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 05/05/2012 13:49 Bruce Evans said the following:
> On Sat, 5 May 2012, Andriy Gapon wrote:
>
>> on 04/05/2012 18:25 John Baldwin said the following:
>>> On Thursday, May 03, 2012 11:23:51 am Andriy Gapon wrote:
on 03/05/2012 18:02 Andriy Gapon said the following:
>
> Here's the latest version of the patches:
> http://people.freebsd.org/~avg/zfsboot.patches.4.diff
I've found a couple of problems in the previous version, so here's another
one:
http://people.freebsd.org/~avg/zfsboot.patches.5.diff
The important change is in the first patch (__exec args).
>>>
>>> A few comments/suggestions on the args bits:
>>
>> John,
>>
>> these are excellent suggestions! Thank you!
>> Some comments:
>>> - Add #ifndef LOCORE guards to the new header around the structure so
>>> it can be used in assembly as well as C.
>>
>> Done. I have had to go into a few btx makefiles and add a necessary include
>> path and -DLOCORE to make the header usable from asm.
Bruce,
first a note that the change that we discussed affects (should affect) only BTX
code and as such only boot1/2 -> loader interface.
> Ugh, why not use genassym, as is done for all old uses of this header in
> locore.s, at least on i386 (5% of the i386 genassym.c is for this).
Can not parse 'this header' in this context. We were talking about a new header
file, so there could not be any old uses of it :-)
Probably you meant sys/i386/include/bootinfo.h ?
But, as you say later, it's probably not easy to use genassym with sys/boot
code. Not sure if it would be worth while going this path given the possible
alternatives.
>>> - Move BI_SIZE and ARGOFF into the header as constants.
>>
>> Done.
>>
>>> - Add a CTASSERT() in loader/main.c that BI_SIZE == sizeof(struct bootinfo)
>
> Ugh, BI_SIZE was already used in locore.s.
OK, but this is "the other" BI_SIZE. Maybe the name clash is not nice indeed,
though.
> It wasn't the size of the struct,
> but was the offset of the field that gives the size. No CTASSERT() was
> needed -- the size is whatever it is, as given by sizeof() on the struct
> at the time of compilation of the utility that initializes the struct.
> It was a feature that sizeof() and offsetof() can't be used in asm so they
> must be translated in genassym and no macros are needed in the header (the
> size was fully dynamic, so the asm code only needs the offsetof() values).
> Of course, you could use CTASSERT()s to check that the struct layout didn't
> get broken. The old code just assumes that the struct is packed by the
> programmer and that the arch's struct packing conventions don't change,
> so that for example BI_SIZE = offsetof(struct bootinfo, bi_size) never
> changes.
It seems that boot1/2 -> kernel interface and boo1/2 -> {btxldr, btx} -> loader
interfaces are quite independent and a bit different.
> genassym is hard to use in boot programs, but the old design was that
> boot programs shouldn't use bootinfo in asm and should just use the
> target bootinfo.h at compile time (whatever time the target is compiled).
I am not sure if it is worthwhile adapting genassym to sys/boot...
BTX code needs to know only "some size" of bootinfo. Although it doesn't look
like boot1/2 passes anything really useful to loader via bootinfo except for
bi_bios_dev. For that matter it looks like maybe only two fields from the whole
(x86) bootinfo are useful to (x86) kernel either...
> Anyway, LOCORE means "for use in locore.[sS]", so other uses of it, e.g.
> in boot programs, are bogus.
That's a good point. Maybe we should use some more generic name. Maybe there
is even some macro that is always set for .S files that we can check. Oh, thank
google, is __ASSEMBLER__ it?
It seems like couple of non-x86 headers already use this macro.
>> I have added a definition of CTASSERT to boostrap.h as it was not available
>> for
>> sys/boot and there were two local definitions of the macro in individual
>> files.
>>
>> However the assertion would fail right now.
>> The backward-compatible value of BI_SIZE (72 == 0x48) covers only part of the
>
> This isn't backwards compatible. BI_SIZE was decimal 48 (covers everything
> up to the bi_size field).
I meant backward compatible with the BTX code that I was changing, of course.
>> fields in struct bootinfo, those up to the following comment:
>> /* Items below only from advanced bootloader */
>> I am a little bit hesitant: should I increase BI_SIZE to cover the whole
>> struct
>> bootinfo or should I compare BI_SIZE to offsetof bi_kernend?
>
> Neither. BI_SIZE shouldn't exist. It defeats the bi_size field.
Using the bi_size field may be the proper solution indeed. Even if no data
beyond certain offset is ever used by loader. The planned changes to BTX code
should make using bi_size easier.
>>> Maybe
>>> create a 'struct kargs_ext' that looks like this:
>>>
>>> struct kargs_ext {
>>> uint32_t size;
>>> char data[0];
>>> };
>>
>> I've decided to skip
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Saturday, May 05, 2012 5:53:07 am Andriy Gapon wrote: > on 05/05/2012 12:31 Andriy Gapon said the following: > > on 04/05/2012 18:25 John Baldwin said the following: > >> On Thursday, May 03, 2012 11:23:51 am Andriy Gapon wrote: > >>> on 03/05/2012 18:02 Andriy Gapon said the following: > > Here's the latest version of the patches: > http://people.freebsd.org/~avg/zfsboot.patches.4.diff > >>> > >>> I've found a couple of problems in the previous version, so here's > >>> another one: > >>> http://people.freebsd.org/~avg/zfsboot.patches.5.diff > >>> The important change is in the first patch (__exec args). > >> > >> A few comments/suggestions on the args bits: > > > > John, > > > > these are excellent suggestions! Thank you! > > The new patchset: http://people.freebsd.org/~avg/zfsboot.patches.7.diff Looks great, thanks! A few replies below: > >> - Add a CTASSERT() in loader/main.c that BI_SIZE == sizeof(struct bootinfo) > > > > I have added a definition of CTASSERT to boostrap.h as it was not available > > for > > sys/boot and there were two local definitions of the macro in individual > > files. > > > > However the assertion would fail right now. > > The backward-compatible value of BI_SIZE (72 == 0x48) covers only part of > > the > > fields in struct bootinfo, those up to the following comment: > > /* Items below only from advanced bootloader */ > > > > I am a little bit hesitant: should I increase BI_SIZE to cover the whole > > struct > > bootinfo or should I compare BI_SIZE to offsetof bi_kernend? Actually, we should probably be reading the 'bi_size' field and not using a BI_SIZE constant at all? Looks like only the non-functional EFI boot loader doesn't set bi_size (and it should just be fixed to do so since it needs to pass new fields in anyway). > > I've decided to define ARGADJ in the new common header, then I've had to > > rename > > btxcsu.s to .S, so that the preprocessing is executed for it. Ok. Maybe add one comment to the bootargs.h head to explain that the 'bootargs' struct starts at ARGOFF and can grow up, while struct bootinfo is copied such that it's end is at the top of the argument area and grows down. Also, at some point we could use a genassym.c file ala the kernel builds to generate some of the constants in bootargs.h instead (e.g. the offsets of fields within structures, and BA_SIZE, though we probably want to ensure that BA_SIZE never changes). -- John Baldwin ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Sat, 5 May 2012, Andriy Gapon wrote:
on 04/05/2012 18:25 John Baldwin said the following:
On Thursday, May 03, 2012 11:23:51 am Andriy Gapon wrote:
on 03/05/2012 18:02 Andriy Gapon said the following:
Here's the latest version of the patches:
http://people.freebsd.org/~avg/zfsboot.patches.4.diff
I've found a couple of problems in the previous version, so here's another one:
http://people.freebsd.org/~avg/zfsboot.patches.5.diff
The important change is in the first patch (__exec args).
A few comments/suggestions on the args bits:
John,
these are excellent suggestions! Thank you!
Some comments:
- Add #ifndef LOCORE guards to the new header around the structure so
it can be used in assembly as well as C.
Done. I have had to go into a few btx makefiles and add a necessary include
path and -DLOCORE to make the header usable from asm.
Ugh, why not use genassym, as is done for all old uses of this header in
locore.s, at least on i386 (5% of the i386 genassym.c is for this).
- Move BI_SIZE and ARGOFF into the header as constants.
Done.
- Add a CTASSERT() in loader/main.c that BI_SIZE == sizeof(struct bootinfo)
Ugh, BI_SIZE was already used in locore.s. It wasn't the size of the struct,
but was the offset of the field that gives the size. No CTASSERT() was
needed -- the size is whatever it is, as given by sizeof() on the struct
at the time of compilation of the utility that initializes the struct.
It was a feature that sizeof() and offsetof() can't be used in asm so they
must be translated in genassym and no macros are needed in the header (the
size was fully dynamic, so the asm code only needs the offsetof() values).
Of course, you could use CTASSERT()s to check that the struct layout didn't
get broken. The old code just assumes that the struct is packed by the
programmer and that the arch's struct packing conventions don't change,
so that for example BI_SIZE = offsetof(struct bootinfo, bi_size) never
changes.
genassym is hard to use in boot programs, but the old design was that
boot programs shouldn't use bootinfo in asm and should just use the
target bootinfo.h at compile time (whatever time the target is compiled).
Anyway, LOCORE means "for use in locore.[sS]", so other uses of it, e.g.
in boot programs, are bogus.
I have added a definition of CTASSERT to boostrap.h as it was not available for
sys/boot and there were two local definitions of the macro in individual files.
However the assertion would fail right now.
The backward-compatible value of BI_SIZE (72 == 0x48) covers only part of the
This isn't backwards compatible. BI_SIZE was decimal 48 (covers everything
up to the bi_size field).
fields in struct bootinfo, those up to the following comment:
/* Items below only from advanced bootloader */
I am a little bit hesitant: should I increase BI_SIZE to cover the whole struct
bootinfo or should I compare BI_SIZE to offsetof bi_kernend?
Neither. BI_SIZE shouldn't exist. It defeats the bi_size field.
Maybe
create a 'struct kargs_ext' that looks like this:
struct kargs_ext {
uint32_t size;
char data[0];
};
I've decided to skip on this.
Use KNF indentation and KNF field prefixes (ka_) if you add it :-). Generic
field names like `size' and `data' need prefixes more than mos.
The old struct was:
% #define N_BIOS_GEOM 8
% ...
% /*
% * A zero bootinfo field often means that there is no info available.
% * Flags are used to indicate the validity of fields where zero is a
% * normal value.
% */
% struct bootinfo {
% u_int32_t bi_version;
% u_int32_t bi_kernelname; /* represents a char * */
% u_int32_t bi_nfs_diskless;/* struct nfs_diskless * */
% /* End of fields that are always present. */
The original size was apparently 12.
% #define bi_endcommonbi_n_bios_used
Another style difference. The magic 12 is essentially given by this macro.
This macro is a pseudo-field, like the ones for the copyable and zeroable
regions in struct proc. Its name is in lower case.
% u_int32_t bi_n_bios_used;
% u_int32_t bi_bios_geom[N_BIOS_GEOM];
The struct was broken in 1994 by adding the above 2 fields without providing
any way to distinguish it from the old struct.
% u_int32_t bi_size;
% u_int8_tbi_memsizes_valid;
% u_int8_tbi_bios_dev;/* bootdev BIOS unit number */
% u_int8_tbi_pad[2];
% u_int32_t bi_basemem;
% u_int32_t bi_extmem;
% u_int32_t bi_symtab; /* struct symtab * */
% u_int32_t bi_esymtab; /* struct symtab * */
The above 8 fields were added in 1995 (together with fixing style bugs
like no prefixes for the old field names). Now the struct is determined
by its size according to the bi_size field, and the bi_version field is
not really used (it's much easier to ad
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 05/05/2012 12:31 Andriy Gapon said the following:
> on 04/05/2012 18:25 John Baldwin said the following:
>> On Thursday, May 03, 2012 11:23:51 am Andriy Gapon wrote:
>>> on 03/05/2012 18:02 Andriy Gapon said the following:
Here's the latest version of the patches:
http://people.freebsd.org/~avg/zfsboot.patches.4.diff
>>>
>>> I've found a couple of problems in the previous version, so here's another
>>> one:
>>> http://people.freebsd.org/~avg/zfsboot.patches.5.diff
>>> The important change is in the first patch (__exec args).
>>
>> A few comments/suggestions on the args bits:
>
> John,
>
> these are excellent suggestions! Thank you!
The new patchset: http://people.freebsd.org/~avg/zfsboot.patches.7.diff
> Some comments:
>
>> - Please move the type of the 'kargs' struct into the new header and
>> give it a type name. Don't add the LOADER_ZFS_SUPPORT #ifdef's
>> however or the new size field (see below).
>
> Done. I've named the header and the struct "bootargs".
>
>> - Add #ifndef LOCORE guards to the new header around the structure so
>> it can be used in assembly as well as C.
>
> Done. I have had to go into a few btx makefiles and add a necessary include
> path and -DLOCORE to make the header usable from asm.
>
>> - Move BI_SIZE and ARGOFF into the header as constants.
>
> Done.
>
>> - Add a CTASSERT() in loader/main.c that BI_SIZE == sizeof(struct bootinfo)
>
> I have added a definition of CTASSERT to boostrap.h as it was not available
> for
> sys/boot and there were two local definitions of the macro in individual
> files.
>
> However the assertion would fail right now.
> The backward-compatible value of BI_SIZE (72 == 0x48) covers only part of the
> fields in struct bootinfo, those up to the following comment:
> /* Items below only from advanced bootloader */
>
> I am a little bit hesitant: should I increase BI_SIZE to cover the whole
> struct
> bootinfo or should I compare BI_SIZE to offsetof bi_kernend?
>
>> - Add a KA_SIZE for sizeof(struct kargs) with a CTASSERT. Use it in this
>> instruction:
>
> Done.
>
>> +addl 0x1c(%esp),%ecx# Add size of variable args
>>
>> in place of the 0x1c
>
> Done. Plus some more.
>
>> - Don't add the new 'size' field to the end of the 'struct kargs'. Instead
>> add a comment saying that if KARGS_FLAGS_EXT is set, then a structure will
>> be present at (kargs + 1) that has a int size as its first member.
>
> Done.
>
>> Maybe
>> create a 'struct kargs_ext' that looks like this:
>>
>> struct kargs_ext {
>> uint32_t size;
>> char data[0];
>> };
>
> I've decided to skip on this.
>
>> - Use 'zargs = (struct zfs_boot_args)(kargs + 1)' in loader/main.c.
>> - Change the ARGADJ line in btxcsu to be this:
>>
>> .set ARGADJ,0x1000 - ARGOFF
>
> I've decided to define ARGADJ in the new common header, then I've had to
> rename
> btxcsu.s to .S, so that the preprocessing is executed for it.
>
>> - If you are adventurous, you could even add a new constant ARGSPACE or some
>> such with an appropriate comment in the new header that you use to replace
>> the 0x1000 in btx.S and in the definition of ARGADJ.
>
> Done.
>
>> Hope that isn't too much, but I do think this will help make things even more
>> understandable.
>>
>
> I will send the new patch shortly.
>
--
Andriy Gapon
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 04/05/2012 18:25 John Baldwin said the following:
> On Thursday, May 03, 2012 11:23:51 am Andriy Gapon wrote:
>> on 03/05/2012 18:02 Andriy Gapon said the following:
>>>
>>> Here's the latest version of the patches:
>>> http://people.freebsd.org/~avg/zfsboot.patches.4.diff
>>
>> I've found a couple of problems in the previous version, so here's another
>> one:
>> http://people.freebsd.org/~avg/zfsboot.patches.5.diff
>> The important change is in the first patch (__exec args).
>
> A few comments/suggestions on the args bits:
John,
these are excellent suggestions! Thank you!
Some comments:
> - Please move the type of the 'kargs' struct into the new header and
> give it a type name. Don't add the LOADER_ZFS_SUPPORT #ifdef's
> however or the new size field (see below).
Done. I've named the header and the struct "bootargs".
> - Add #ifndef LOCORE guards to the new header around the structure so
> it can be used in assembly as well as C.
Done. I have had to go into a few btx makefiles and add a necessary include
path and -DLOCORE to make the header usable from asm.
> - Move BI_SIZE and ARGOFF into the header as constants.
Done.
> - Add a CTASSERT() in loader/main.c that BI_SIZE == sizeof(struct bootinfo)
I have added a definition of CTASSERT to boostrap.h as it was not available for
sys/boot and there were two local definitions of the macro in individual files.
However the assertion would fail right now.
The backward-compatible value of BI_SIZE (72 == 0x48) covers only part of the
fields in struct bootinfo, those up to the following comment:
/* Items below only from advanced bootloader */
I am a little bit hesitant: should I increase BI_SIZE to cover the whole struct
bootinfo or should I compare BI_SIZE to offsetof bi_kernend?
> - Add a KA_SIZE for sizeof(struct kargs) with a CTASSERT. Use it in this
> instruction:
Done.
> + addl 0x1c(%esp),%ecx# Add size of variable args
>
> in place of the 0x1c
Done. Plus some more.
> - Don't add the new 'size' field to the end of the 'struct kargs'. Instead
> add a comment saying that if KARGS_FLAGS_EXT is set, then a structure will
> be present at (kargs + 1) that has a int size as its first member.
Done.
> Maybe
> create a 'struct kargs_ext' that looks like this:
>
> struct kargs_ext {
> uint32_t size;
> char data[0];
> };
I've decided to skip on this.
> - Use 'zargs = (struct zfs_boot_args)(kargs + 1)' in loader/main.c.
> - Change the ARGADJ line in btxcsu to be this:
>
> .set ARGADJ,0x1000 - ARGOFF
I've decided to define ARGADJ in the new common header, then I've had to rename
btxcsu.s to .S, so that the preprocessing is executed for it.
> - If you are adventurous, you could even add a new constant ARGSPACE or some
> such with an appropriate comment in the new header that you use to replace
> the 0x1000 in btx.S and in the definition of ARGADJ.
Done.
> Hope that isn't too much, but I do think this will help make things even more
> understandable.
>
I will send the new patch shortly.
--
Andriy Gapon
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Thursday, May 03, 2012 11:23:51 am Andriy Gapon wrote:
> on 03/05/2012 18:02 Andriy Gapon said the following:
> >
> > Here's the latest version of the patches:
> > http://people.freebsd.org/~avg/zfsboot.patches.4.diff
>
> I've found a couple of problems in the previous version, so here's another
> one:
> http://people.freebsd.org/~avg/zfsboot.patches.5.diff
> The important change is in the first patch (__exec args).
A few comments/suggestions on the args bits:
- Please move the type of the 'kargs' struct into the new header and
give it a type name. Don't add the LOADER_ZFS_SUPPORT #ifdef's
however or the new size field (see below).
- Add #ifndef LOCORE guards to the new header around the structure so
it can be used in assembly as well as C.
- Move BI_SIZE and ARGOFF into the header as constants.
- Add a CTASSERT() in loader/main.c that BI_SIZE == sizeof(struct bootinfo)
- Add a KA_SIZE for sizeof(struct kargs) with a CTASSERT. Use it in this
instruction:
+ addl 0x1c(%esp),%ecx# Add size of variable args
in place of the 0x1c
- Don't add the new 'size' field to the end of the 'struct kargs'. Instead
add a comment saying that if KARGS_FLAGS_EXT is set, then a structure will
be present at (kargs + 1) that has a int size as its first member. Maybe
create a 'struct kargs_ext' that looks like this:
struct kargs_ext {
uint32_t size;
char data[0];
};
- Use 'zargs = (struct zfs_boot_args)(kargs + 1)' in loader/main.c.
- Change the ARGADJ line in btxcsu to be this:
.set ARGADJ,0x1000 - ARGOFF
- If you are adventurous, you could even add a new constant ARGSPACE or some
such with an appropriate comment in the new header that you use to replace
the 0x1000 in btx.S and in the definition of ARGADJ.
Hope that isn't too much, but I do think this will help make things even more
understandable.
--
John Baldwin
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 03/05/2012 18:02 Andriy Gapon said the following:
>
> Here's the latest version of the patches:
> http://people.freebsd.org/~avg/zfsboot.patches.4.diff
I've found a couple of problems in the previous version, so here's another one:
http://people.freebsd.org/~avg/zfsboot.patches.5.diff
The important change is in the first patch (__exec args).
> John,
> the first of the patches implements the approach that we previously discussed.
> All arguments are passed starting at a fixed offset that should provide enough
> space for extending argument list. The first of the extended arguments
> should be
> a size of the arguments (including the size field). Then it's easy to write
> something like:
> struct xargs
> {
> uint32_t size;
> ...
> };
> ...
> struct xargs xargs;
> xargs.size = sizeof(xargs);
> ...
> __exec(..., xargs);
>
>
> Marius, Gavin,
> patch 1f94d9a is my attempt of adapting your sparc64 ZFS code to my larger
> changes
> in patch ae5a9c6. I have the patches separate to facilitate the review. They
> should be committed together. I have only compile-tested the sparc64/ofw
> part, so
> it could have some grave bugs or omissions.
> Could you please review and/or test this patch?
> I will greatly appreciate any discussion, suggestions, help.
>
> I again invite everyone else to take part in the review and testing.
>
--
Andriy Gapon
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
Here's the latest version of the patches:
http://people.freebsd.org/~avg/zfsboot.patches.4.diff
John,
the first of the patches implements the approach that we previously discussed.
All arguments are passed starting at a fixed offset that should provide enough
space for extending argument list. The first of the extended arguments should
be
a size of the arguments (including the size field). Then it's easy to write
something like:
struct xargs
{
uint32_t size;
...
};
...
struct xargs xargs;
xargs.size = sizeof(xargs);
...
__exec(..., xargs);
Marius, Gavin,
patch 1f94d9a is my attempt of adapting your sparc64 ZFS code to my larger
changes
in patch ae5a9c6. I have the patches separate to facilitate the review. They
should be committed together. I have only compile-tested the sparc64/ofw part,
so
it could have some grave bugs or omissions.
Could you please review and/or test this patch?
I will greatly appreciate any discussion, suggestions, help.
I again invite everyone else to take part in the review and testing.
--
Andriy Gapon
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Mon, Apr 30, 2012 at 10:04:17AM +0300, Andriy Gapon wrote: > on 29/04/2012 19:46 Marius Strobl said the following: > > Given that you certainly have a well better knowledge of ZFS, it > > would be great if we could do it the other way around, i.e. commit > > the sparc64 support first and then your patch after adapting > > whatever you have in mind with things becoming different. In other > > words, I'm basically ready to commit the following patch. As for > > zfs_dev_init() this just wraps it in #if defined(__amd64__) || > > defined(__i386__) in zfs.c for now. > > http://people.freebsd.org/~marius/boot_zfs_sparc64.diff > > OK, let's do it this way. > Thanks! Marius ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 29/04/2012 19:46 Marius Strobl said the following: > Given that you certainly have a well better knowledge of ZFS, it > would be great if we could do it the other way around, i.e. commit > the sparc64 support first and then your patch after adapting > whatever you have in mind with things becoming different. In other > words, I'm basically ready to commit the following patch. As for > zfs_dev_init() this just wraps it in #if defined(__amd64__) || > defined(__i386__) in zfs.c for now. > http://people.freebsd.org/~marius/boot_zfs_sparc64.diff OK, let's do it this way. -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Fri, Apr 27, 2012 at 12:06:08PM +0300, Andriy Gapon wrote: > on 23/04/2012 00:21 Marius Strobl said the following: > > On Sat, Apr 14, 2012 at 06:37:54PM +0300, Andriy Gapon wrote: > [snip] > >> I am particularly interested in reviews of my attempt to make ZFS boot > >> support > >> arch-independent. The arches, of course, would have to add some code to > >> make > >> use of that support. Currently I only enabled it for x86. > >> > > > > I can't say much about these patches as a whole as they are rather > > big and I'm not aware of all the details of ZFS. However, one bit that > > makes the current implementation x86-specific is zfs_dev_init(). If > > you could move it to the MD part in the course of these patches that > > would be great. > > I have arranged this in my WIP version of the patch, which I hope to share > soon. > Need to work out some unrelated details. > > > If you could also take the second patch in PR 165025 > > into account, which I plan to commit once the issue with the current > > ofw_disk.c are properly solved, that would be great. > > Thank you for the heads up. > Since I also hope to commit my patch rather soon, I would also appreciate if > you > keep my changes in mind :-) > In fact, I would like to ask you if it would make sense to postpone the patch > from the PR until my patch is committed. That should make some things easier > to > do (e.g. MD zfs_dev_init), but on the other hand some things would become > different. > Either way, one of the patches would have to be rebased on top of the other. > Given that you certainly have a well better knowledge of ZFS, it would be great if we could do it the other way around, i.e. commit the sparc64 support first and then your patch after adapting whatever you have in mind with things becoming different. In other words, I'm basically ready to commit the following patch. As for zfs_dev_init() this just wraps it in #if defined(__amd64__) || defined(__i386__) in zfs.c for now. http://people.freebsd.org/~marius/boot_zfs_sparc64.diff Marius ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 27/04/2012 13:37 Andrey V. Elsukov said the following: > What you think about this concept: > We can implement some MI API to query disks count and each disk parameters > (mediasize, > sectorsize). This MI code will use some IOCTL that will act with MD "disk"'s > devsw->ioctl. > > devicename.c functions can be changed to use MI DISK API together with PART > API, > or maybe even moved to the MI code. > > In the result we will have some MI API to get access to the disks and > partitions, > that we can use anywhere, e.g. in the ZFS code. Sorry, but I couldn't understand your design. Probably I am missing the bigger picture. I think that for ZFS case it would be sufficient to be able to iterate over detected device names or devspecs. Size and other disk properties do not seem to be needed. Unless I am mistaken, of course. E.g. something akin to what bd_print (dv_print, in the general case) does but oriented towards programmatic use rather than end-user. Maybe all that partition table parsing should be done only once (e.g. in the init method) and the result should be saved. -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On 27.04.2012 13:14, Andriy Gapon wrote: >> I also have some WIP related to moving partition table handling into MD Oh, i did mean MI part. >> part. You can look here: http://people.freebsd.org/~ae/sys_boot.diff > > I like this patch. OTOH, I couldn't help by wonder if it is possible to > somehow directly re-use the results of dv_init probing by other drivers (or > subset of the drivers, like only 'disk'). E.g. if instead of re-examining the > partition tables, we could ask a driver for a list of devices that it > discovered. > But how to do that is not something that I can answer. What you think about this concept: We can implement some MI API to query disks count and each disk parameters (mediasize, sectorsize). This MI code will use some IOCTL that will act with MD "disk"'s devsw->ioctl. devicename.c functions can be changed to use MI DISK API together with PART API, or maybe even moved to the MI code. In the result we will have some MI API to get access to the disks and partitions, that we can use anywhere, e.g. in the ZFS code. -- WBR, Andrey V. Elsukov ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 23/04/2012 09:23 Andrey V. Elsukov said the following: > On 23.04.2012 1:21, Marius Strobl wrote: >> I can't say much about these patches as a whole as they are rather big >> and I'm not aware of all the details of ZFS. However, one bit that makes >> the current implementation x86-specific is zfs_dev_init(). If you could >> move it to the MD part in the course of these patches that would be >> great. If you could also take the second patch in PR 165025 into account, >> which I plan to commit once the issue with the current ofw_disk.c are >> properly solved, that would be great. > > I also have some WIP related to moving partition table handling into MD > part. You can look here: http://people.freebsd.org/~ae/sys_boot.diff I like this patch. OTOH, I couldn't help by wonder if it is possible to somehow directly re-use the results of dv_init probing by other drivers (or subset of the drivers, like only 'disk'). E.g. if instead of re-examining the partition tables, we could ask a driver for a list of devices that it discovered. But how to do that is not something that I can answer. > This patch have one problem, there is no way to determine disk size and i'm > thinking about adding ioctl(DIOCGMEDIASIZE) to the "disk" devsw. > -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 23/04/2012 00:21 Marius Strobl said the following: > On Sat, Apr 14, 2012 at 06:37:54PM +0300, Andriy Gapon wrote: [snip] >> I am particularly interested in reviews of my attempt to make ZFS boot >> support >> arch-independent. The arches, of course, would have to add some code to make >> use of that support. Currently I only enabled it for x86. >> > > I can't say much about these patches as a whole as they are rather > big and I'm not aware of all the details of ZFS. However, one bit that > makes the current implementation x86-specific is zfs_dev_init(). If > you could move it to the MD part in the course of these patches that > would be great. I have arranged this in my WIP version of the patch, which I hope to share soon. Need to work out some unrelated details. > If you could also take the second patch in PR 165025 > into account, which I plan to commit once the issue with the current > ofw_disk.c are properly solved, that would be great. Thank you for the heads up. Since I also hope to commit my patch rather soon, I would also appreciate if you keep my changes in mind :-) In fact, I would like to ask you if it would make sense to postpone the patch from the PR until my patch is committed. That should make some things easier to do (e.g. MD zfs_dev_init), but on the other hand some things would become different. Either way, one of the patches would have to be rebased on top of the other. -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On 23.04.2012 1:21, Marius Strobl wrote: > I can't say much about these patches as a whole as they are rather > big and I'm not aware of all the details of ZFS. However, one bit that > makes the current implementation x86-specific is zfs_dev_init(). If > you could move it to the MD part in the course of these patches that > would be great. If you could also take the second patch in PR 165025 > into account, which I plan to commit once the issue with the current > ofw_disk.c are properly solved, that would be great. I also have some WIP related to moving partition table handling into MD part. You can look here: http://people.freebsd.org/~ae/sys_boot.diff This patch have one problem, there is no way to determine disk size and i'm thinking about adding ioctl(DIOCGMEDIASIZE) to the "disk" devsw. -- WBR, Andrey V. Elsukov signature.asc Description: OpenPGP digital signature
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Sat, Apr 14, 2012 at 06:37:54PM +0300, Andriy Gapon wrote: > > I would like to ask for a review and/or testing of the following three > patches: > http://people.freebsd.org/~avg/zfsboot.patches.diff > > These patches add support for booting from an arbitrary filesystem of any > detected ZFS pool. A filesystem could be selected in zfsboot and thus will > affectfrom where zfsloader would be loaded. zfsboot passes information about > the boot pool and filesystem to zfsloader, which uses those for loaddev and > default value of currdev. A different pool+filesystem could be selected in > zfsloader for booting kernel. Also if vfs.root.mountfrom is not explicitly > set > and is not derived from fstab, then it gets set to the selected boot > filesystem. > > This should could be used as a foundation for the support of Solaris-like boot > environment selection. I believe that other people have already developed > scripts utilizing ZFS capabilities to provide other aspects of management of > boot environments. > > I am particularly interested in reviews of my attempt to make ZFS boot support > arch-independent. The arches, of course, would have to add some code to make > use of that support. Currently I only enabled it for x86. > I can't say much about these patches as a whole as they are rather big and I'm not aware of all the details of ZFS. However, one bit that makes the current implementation x86-specific is zfs_dev_init(). If you could move it to the MD part in the course of these patches that would be great. If you could also take the second patch in PR 165025 into account, which I plan to commit once the issue with the current ofw_disk.c are properly solved, that would be great. Marius ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Wednesday, April 18, 2012 11:00:18 am Andriy Gapon wrote: > on 18/04/2012 17:40 Ian Lepore said the following: > > On Wed, 2012-04-18 at 17:36 +0300, Andriy Gapon wrote: > >> on 18/04/2012 17:22 Ian Lepore said the following: > >>> YES! A size field (preferably as the first field in the struct) along > >>> with a flag to indicate that it's a new-style boot info struct that > >>> starts with a size field, will allow future changes without a lot of > >>> drama. It can allow code that has to deal with the struct without > >>> interpretting it (such as trampoline code that has to copy it to a new > >>> stack or memory area as part of loading the kernel) to be immune to > >>> future changes. > >> > >> Yeah, placing the new field at front would immediately break compatibility > >> and > >> even access to the flags field :-) > >> > > > > Code would only assume the new field was at the front of the struct if > > the new flag is set, otherwise it would use the historical struct > > layout. > > Right, but where the flag would reside? > And how the older code that is not aware of the new flag would cope with the > new > layout? I think the size should be appended to the end of the current structure. However, it will buy us more flexibility in the future. -- John Baldwin ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 18/04/2012 17:40 Ian Lepore said the following: > On Wed, 2012-04-18 at 17:36 +0300, Andriy Gapon wrote: >> on 18/04/2012 17:22 Ian Lepore said the following: >>> YES! A size field (preferably as the first field in the struct) along >>> with a flag to indicate that it's a new-style boot info struct that >>> starts with a size field, will allow future changes without a lot of >>> drama. It can allow code that has to deal with the struct without >>> interpretting it (such as trampoline code that has to copy it to a new >>> stack or memory area as part of loading the kernel) to be immune to >>> future changes. >> >> Yeah, placing the new field at front would immediately break compatibility >> and >> even access to the flags field :-) >> > > Code would only assume the new field was at the front of the struct if > the new flag is set, otherwise it would use the historical struct > layout. Right, but where the flag would reside? And how the older code that is not aware of the new flag would cope with the new layout? -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Wed, 2012-04-18 at 17:36 +0300, Andriy Gapon wrote: > on 18/04/2012 17:22 Ian Lepore said the following: > > YES! A size field (preferably as the first field in the struct) along > > with a flag to indicate that it's a new-style boot info struct that > > starts with a size field, will allow future changes without a lot of > > drama. It can allow code that has to deal with the struct without > > interpretting it (such as trampoline code that has to copy it to a new > > stack or memory area as part of loading the kernel) to be immune to > > future changes. > > Yeah, placing the new field at front would immediately break compatibility and > even access to the flags field :-) > Oh wait, is the flags field embedded in the struct? My bad, I didn't look. In the ARM code I'm used to working with, the flags are passed from the bootloader to the kernel entry point in a register; I don't know why assumed that would be true on other platforms. -- Ian ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Wed, 2012-04-18 at 17:36 +0300, Andriy Gapon wrote: > on 18/04/2012 17:22 Ian Lepore said the following: > > YES! A size field (preferably as the first field in the struct) along > > with a flag to indicate that it's a new-style boot info struct that > > starts with a size field, will allow future changes without a lot of > > drama. It can allow code that has to deal with the struct without > > interpretting it (such as trampoline code that has to copy it to a new > > stack or memory area as part of loading the kernel) to be immune to > > future changes. > > Yeah, placing the new field at front would immediately break compatibility and > even access to the flags field :-) > Code would only assume the new field was at the front of the struct if the new flag is set, otherwise it would use the historical struct layout. -- Ian ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 18/04/2012 17:22 Ian Lepore said the following: > YES! A size field (preferably as the first field in the struct) along > with a flag to indicate that it's a new-style boot info struct that > starts with a size field, will allow future changes without a lot of > drama. It can allow code that has to deal with the struct without > interpretting it (such as trampoline code that has to copy it to a new > stack or memory area as part of loading the kernel) to be immune to > future changes. Yeah, placing the new field at front would immediately break compatibility and even access to the flags field :-) -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Wed, 2012-04-18 at 09:41 -0400, John Baldwin wrote: > On Wednesday, April 18, 2012 2:02:22 am Andriy Gapon wrote: > > on 17/04/2012 23:43 John Baldwin said the following: > > > On Tuesday, April 17, 2012 4:22:19 pm Andriy Gapon wrote: > > >> We already have a flag for ZFS (KARGS_FLAGS_ZFS, 0x4). So the new flag > > >> could be > > >> named something ZFS-specific (as silly as KARGS_FLAGS_ZFS2) or something > > >> more > > >> general such as KARGS_FLAGS_32_BYTES meaning that the total size of > > >> arguments > > >> area is 32 bytes (as opposed to 24 previously). > > > > > > Does KARGS_FLAGS_GUID work? > > > > > > > I think that's too terse, we already passed a pool guid via the existing > > argument space. So it should be something like KARGS_FLAGS_ZFS_FS_GUID or > > KARGS_FLAGS_ZFS_DS_GUID (DS - dataset). > > Ah. I do think the flag should indicate that the bootinfo structure is > larger, > I was assuming you were adding a new GUID field that didn't exist before. > I can't think of something better than KARGS_FLAGS_32. What might be nice > actually, is to add a new field to indicate the size of the argument area and > to set a flag to indicate that the size field is present (KARGS_FLAGS_SIZE)? YES! A size field (preferably as the first field in the struct) along with a flag to indicate that it's a new-style boot info struct that starts with a size field, will allow future changes without a lot of drama. It can allow code that has to deal with the struct without interpretting it (such as trampoline code that has to copy it to a new stack or memory area as part of loading the kernel) to be immune to future changes. This probably isn't a big deal in the x86 world, but it can be important for embedded systems where a proprietary bootloader has to pass info to a proprietary board_init() type routine in the kernel using non-proprietary loader/trampoline code that's part of the base. We have a bit of a mess in this regard in the ARM world right now, and it would be a lot lessy messy if something like this had been in place. -- Ian ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Wednesday, April 18, 2012 2:02:22 am Andriy Gapon wrote: > on 17/04/2012 23:43 John Baldwin said the following: > > On Tuesday, April 17, 2012 4:22:19 pm Andriy Gapon wrote: > >> We already have a flag for ZFS (KARGS_FLAGS_ZFS, 0x4). So the new flag > >> could be > >> named something ZFS-specific (as silly as KARGS_FLAGS_ZFS2) or something > >> more > >> general such as KARGS_FLAGS_32_BYTES meaning that the total size of > >> arguments > >> area is 32 bytes (as opposed to 24 previously). > > > > Does KARGS_FLAGS_GUID work? > > > > I think that's too terse, we already passed a pool guid via the existing > argument space. So it should be something like KARGS_FLAGS_ZFS_FS_GUID or > KARGS_FLAGS_ZFS_DS_GUID (DS - dataset). Ah. I do think the flag should indicate that the bootinfo structure is larger, I was assuming you were adding a new GUID field that didn't exist before. I can't think of something better than KARGS_FLAGS_32. What might be nice actually, is to add a new field to indicate the size of the argument area and to set a flag to indicate that the size field is present (KARGS_FLAGS_SIZE)? Hmm, looks like we should name this structure and move it and the relevant KARGS_FLAGS_* fields into a header while we are at it? -- John Baldwin ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 15/04/2012 04:01 Bob Friesenhahn said the following: > It would be nice if the updated FreeBSD bootloader could have the ability to > boot both Solaris and FreeBSD root filesystems in the same pool so that one > could switch between several zfs-based operating systems without needing to > use > a different partition for each one. Is this within the bounds of possibility > or > a totally irrational thought? I can not assess feasibility of such a project. Just want to note that ultimately it's the code that gets booted, not filesystems. I have an impression that Solaris boot archive is quite different from how FreeBSD kernel gets booted. -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 14/04/2012 18:37 Andriy Gapon said the following: > > I would like to ask for a review and/or testing of the following three > patches: > http://people.freebsd.org/~avg/zfsboot.patches.diff I've put a new version of the patch here: http://people.freebsd.org/~avg/zfsboot.patches.2.diff Most prominent changes: - new zfsloader should be compatible with previous zfsboot - libi386 unconditionally includes zfs support via use of weak symbols for some functions Unfortunately, unconditional compatibility between i386_devdesc and zfs_devdesc means that i386_devdesc becomes much larger (> 512 bytes). But I think that it shouldn't cause any real problems. > These patches add support for booting from an arbitrary filesystem of any > detected ZFS pool. A filesystem could be selected in zfsboot and thus will > affectfrom where zfsloader would be loaded. zfsboot passes information about > the boot pool and filesystem to zfsloader, which uses those for loaddev and > default value of currdev. A different pool+filesystem could be selected in > zfsloader for booting kernel. Also if vfs.root.mountfrom is not explicitly > set > and is not derived from fstab, then it gets set to the selected boot > filesystem. > > This should could be used as a foundation for the support of Solaris-like boot > environment selection. I believe that other people have already developed > scripts utilizing ZFS capabilities to provide other aspects of management of > boot environments. > > I am particularly interested in reviews of my attempt to make ZFS boot support > arch-independent. The arches, of course, would have to add some code to make > use of that support. Currently I only enabled it for x86. > > Thank you very much! -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 17/04/2012 23:43 John Baldwin said the following: > On Tuesday, April 17, 2012 4:22:19 pm Andriy Gapon wrote: >> We already have a flag for ZFS (KARGS_FLAGS_ZFS, 0x4). So the new flag >> could be >> named something ZFS-specific (as silly as KARGS_FLAGS_ZFS2) or something more >> general such as KARGS_FLAGS_32_BYTES meaning that the total size of arguments >> area is 32 bytes (as opposed to 24 previously). > > Does KARGS_FLAGS_GUID work? > I think that's too terse, we already passed a pool guid via the existing argument space. So it should be something like KARGS_FLAGS_ZFS_FS_GUID or KARGS_FLAGS_ZFS_DS_GUID (DS - dataset). -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Tuesday, April 17, 2012 4:22:19 pm Andriy Gapon wrote: > on 16/04/2012 16:56 John Baldwin said the following: > > On Saturday, April 14, 2012 1:35:35 pm Andriy Gapon wrote: > >> on 14/04/2012 18:37 Andriy Gapon said the following: > >>> > >>> I would like to ask for a review and/or testing of the following three > >>> patches: > >>> http://people.freebsd.org/~avg/zfsboot.patches.diff > >>> > >>> These patches add support for booting from an arbitrary filesystem of any > >>> detected ZFS pool. A filesystem could be selected in zfsboot and thus > >>> will > >>> affectfrom where zfsloader would be loaded. zfsboot passes information > >>> about > >>> the boot pool and filesystem to zfsloader, which uses those for loaddev > >>> and > >>> default value of currdev. A different pool+filesystem could be selected > >>> in > >>> zfsloader for booting kernel. Also if vfs.root.mountfrom is not > >>> explicitly set > >>> and is not derived from fstab, then it gets set to the selected boot > >>> filesystem. > >> > >> A note for prospective testers: the patched loader expect to be started by > >> the > >> patched zfs boot as it passes an additional parameter for a filesystem > >> guid. > >> I should probably add some way to distinguish between the older and newer > >> zfs > >> boot blocks. Maybe an extra bit in bootflags? What do you think? > > > > An extra bit (similar to existing flags for detecting PXE and CD booting) > > sounds fine to me. (Note, I'm only replying to the question, have not > > looked > > at patches yet). > > I hope that you will :-) > > We already have a flag for ZFS (KARGS_FLAGS_ZFS, 0x4). So the new flag could > be > named something ZFS-specific (as silly as KARGS_FLAGS_ZFS2) or something more > general such as KARGS_FLAGS_32_BYTES meaning that the total size of arguments > area is 32 bytes (as opposed to 24 previously). Does KARGS_FLAGS_GUID work? -- John Baldwin ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 16/04/2012 16:56 John Baldwin said the following: > On Saturday, April 14, 2012 1:35:35 pm Andriy Gapon wrote: >> on 14/04/2012 18:37 Andriy Gapon said the following: >>> >>> I would like to ask for a review and/or testing of the following three >>> patches: >>> http://people.freebsd.org/~avg/zfsboot.patches.diff >>> >>> These patches add support for booting from an arbitrary filesystem of any >>> detected ZFS pool. A filesystem could be selected in zfsboot and thus will >>> affectfrom where zfsloader would be loaded. zfsboot passes information >>> about >>> the boot pool and filesystem to zfsloader, which uses those for loaddev and >>> default value of currdev. A different pool+filesystem could be selected in >>> zfsloader for booting kernel. Also if vfs.root.mountfrom is not explicitly >>> set >>> and is not derived from fstab, then it gets set to the selected boot >>> filesystem. >> >> A note for prospective testers: the patched loader expect to be started by >> the >> patched zfs boot as it passes an additional parameter for a filesystem guid. >> I should probably add some way to distinguish between the older and newer zfs >> boot blocks. Maybe an extra bit in bootflags? What do you think? > > An extra bit (similar to existing flags for detecting PXE and CD booting) > sounds fine to me. (Note, I'm only replying to the question, have not looked > at patches yet). I hope that you will :-) We already have a flag for ZFS (KARGS_FLAGS_ZFS, 0x4). So the new flag could be named something ZFS-specific (as silly as KARGS_FLAGS_ZFS2) or something more general such as KARGS_FLAGS_32_BYTES meaning that the total size of arguments area is 32 bytes (as opposed to 24 previously). -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Saturday, April 14, 2012 1:35:35 pm Andriy Gapon wrote: > on 14/04/2012 18:37 Andriy Gapon said the following: > > > > I would like to ask for a review and/or testing of the following three > > patches: > > http://people.freebsd.org/~avg/zfsboot.patches.diff > > > > These patches add support for booting from an arbitrary filesystem of any > > detected ZFS pool. A filesystem could be selected in zfsboot and thus will > > affectfrom where zfsloader would be loaded. zfsboot passes information > > about > > the boot pool and filesystem to zfsloader, which uses those for loaddev and > > default value of currdev. A different pool+filesystem could be selected in > > zfsloader for booting kernel. Also if vfs.root.mountfrom is not explicitly > > set > > and is not derived from fstab, then it gets set to the selected boot > > filesystem. > > A note for prospective testers: the patched loader expect to be started by the > patched zfs boot as it passes an additional parameter for a filesystem guid. > I should probably add some way to distinguish between the older and newer zfs > boot blocks. Maybe an extra bit in bootflags? What do you think? An extra bit (similar to existing flags for detecting PXE and CD booting) sounds fine to me. (Note, I'm only replying to the question, have not looked at patches yet). -- John Baldwin ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
On Apr 14, 2012, at 6:01 PM, Bob Friesenhahn wrote: > It would be nice if the updated FreeBSD bootloader could have the ability to > boot both Solaris and FreeBSD root filesystems in the same pool so that one > could switch between several zfs-based operating systems without needing to > use a different partition for each one. Is this within the bounds of > possibility or a totally irrational thought? This sort of depends on whether or not the device is hardcoded in the zpool, right? Thanks! -Garrett___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
It would be nice if the updated FreeBSD bootloader could have the ability to boot both Solaris and FreeBSD root filesystems in the same pool so that one could switch between several zfs-based operating systems without needing to use a different partition for each one. Is this within the bounds of possibility or a totally irrational thought? Bob -- Bob Friesenhahn bfrie...@simple.dallas.tx.us, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer,http://www.GraphicsMagick.org/ ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
on 14/04/2012 18:37 Andriy Gapon said the following: > > I would like to ask for a review and/or testing of the following three > patches: > http://people.freebsd.org/~avg/zfsboot.patches.diff > > These patches add support for booting from an arbitrary filesystem of any > detected ZFS pool. A filesystem could be selected in zfsboot and thus will > affectfrom where zfsloader would be loaded. zfsboot passes information about > the boot pool and filesystem to zfsloader, which uses those for loaddev and > default value of currdev. A different pool+filesystem could be selected in > zfsloader for booting kernel. Also if vfs.root.mountfrom is not explicitly > set > and is not derived from fstab, then it gets set to the selected boot > filesystem. A note for prospective testers: the patched loader expect to be started by the patched zfs boot as it passes an additional parameter for a filesystem guid. I should probably add some way to distinguish between the older and newer zfs boot blocks. Maybe an extra bit in bootflags? What do you think? > This should could be used as a foundation for the support of Solaris-like boot > environment selection. I believe that other people have already developed > scripts utilizing ZFS capabilities to provide other aspects of management of > boot environments. > > I am particularly interested in reviews of my attempt to make ZFS boot support > arch-independent. The arches, of course, would have to add some code to make > use of that support. Currently I only enabled it for x86. > > Thank you very much! -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
