Re: How to go on with ISO 9660 large file support ?

["Thomas Schmitt" <scdbackup%gmx.net@localhost>]

Hi,

> New code should use kmem(9) for variable-sized or one-time
> allocations,

Currently it is running on kmem(9).
There is old malloc(9) usage in cd9660 which i tried to mimic
until i came to the deprecation statement in the man page.

Shall i change the old malloc(9) usage, too ?
Or in a later PR, maybe ?


> For the most part, struct iso_node is private to the kernel
> implementation of cd9660.  

Regrettably there is a #ifdef _KERNEL in cd9660_node.h
and struct iso_node is defined outside of that area.


> Since fstat(1) and pmap(1) use it, you
> should avoid changing the offsets within the structure of the members
> that fstat(1) and pmap(1) use,

I hope to have achieved that on all non-128-bit machine types by:

  struct iso_file_section {
        unsigned long isofsc_extent;    /* will not be used by cd9660 */
        unsigned long isofsc_size;      /* byte count */
        unsigned long isofsc_start;     /* block address */
  };
  union iso_file_data {
        struct iso_file_section single; /* used if only one extent exists:
                                         * CD9660_FSECT_FROM_INO(.i_number)==1
                                         */
        struct iso_file_section *many;  /* allocated with enough array elements
                                         * for CD9660_FSECT_FROM_INO(.i_number)
                                         * if this is larger than 1.
                                         */
  };
  ...
  struct iso_node {
        ...
  #ifdef CD9660_MULTI_EXTENT
        union iso_file_data iso_fsect;  /* File sections. Their number is given
                                         * by CD9660_FSECT_FROM_INO(.i_number)
                                         */
  #else
        unsigned long iso_extent;       /* extent of file */
        unsigned long i_size;
        unsigned long iso_start;                /* actual start of data of file 
(may be different */
                                /* from iso_extent, if file has extended 
attributes) */
  #endif /* ! CD9660_MULTI_EXTENT */
        ...
  };


> Some day, we ought to find a better way to do what fstat(1) and
> pmap(1) are doing with kernel guts.

At least they should make sure that the binary and the kernel
are compatible. As is done with dynamic linking and .so numbers.


> It would be nice if you could integrate your tests into the atf file
> system tests under src/tests/fs so that they get run automatically.

I promise to learn what's needed and to finally provide such
a test. As said, currently it is neither complete nor easily
portable to a different computer.


> This sounds bad (but not at all surprising).  If the `interesting
> effects' are anything more than harmless messages printed to the
> console and unreadable images -- e.g., programs crash, kernel panics
> or discloses private memory to userland, &c. -- it would be nice if
> you could identify little fixes or at least early checks that can be
> easily back-ported.

It's inbetween: Confusing to userland because file names get shown
twice or more, bear all the same attributes and content, although
the causing files do differ. That's why i propose to drop duplicate
names in favor of a probably best and youngest survivor.

No crashes yet.
The NetBSD-6.1.3 bug of kern/48787 is reported and fixed.

PR 48815 is still open. It shows behavior in the above confusion
class. I.e. ISO files hardly usable but rest of system still healthy.


> > The hardest reason why this information has to be encoded in ino_t
> > is the desire to implement method VFS_VGET(9).

> NFS uses VFS_VGET, so it would be good to continue to support that.

Isn't it rather using VFS_FHTOVP(9) ?
Currently its implementation cd9660_fhtovp() indeed uses VFS_VGET(),
but it could as well use other means to identify an iso_node and
its boss vnode.

The address encoding in ino_t would be a short-sighted hack, if not
NetBSD was far-sighted enough to have 64 bit ino_t.
It even provides enough bits to work around a shortcomming in the
list format of ISO 9660 directory records.

Nevertheless, after bin/48799 and applying my large-file proposal,
you will get to see things like this:

  netbsd# ls -li /mnt/iso/my
  total 8455812
  281479306324160 -r--------  1 thomas  dbus  4329375744 May  6 15:30 large_file

(Inode number in hex = 0x100010210a8c0 :
 A file with two sections. Directory records located above 4 GiB.
 Only 49 of 61 possible bits used.)


> But embedding an ino_t in struct iso_node is probably the easiest way
> to do it, and will avoid breaking fstat(1) and pmap(1).

A unique inode number is a must-have. But for that, ISO 9660
would not need 64 bit.

pmap(1) will not be affected by the API change, because it is
only interested in members of iso_node before the ones which
have to change.

fstat(1) is reading the size (of the first file section),
which still works well on i386 with the changed kernel,
but would fail to compile with the new cd9660_node.h.
(I plan to fix this, of course.)
It is also interpreting the ISO_RRIP_NODE which is located at
the end of struct iso_node. So prone to shifts by 128 bit
pointers on systes with 32 bit int ... theoretically.
(See /usr/src/usr.bin/fstat/isofs.c)

I could avoid both problems by appending a new pointer to
iso_node and keeping the other members as they are.
4 or 8 bytes per node ... or theoretically 16. :))


Have a nice day :)