Re: remote fileio: st_ino truncated to 32 bits in vFile:stat/lstat

[Andrew Burgess <aburgess@redhat.com>]

Abhay Kandpal <abhay@linux.ibm.com> writes:

> Hi Andrew,
>
> I hit a failure in gdb.server/fileio-packets.exp on a machine whose filesystem uses inode numbers above 2^32.
>
> All four stat/lstat checks fail, with every field matching except st_ino:
>
> remote = {..., 'st_ino': 2161233200, ...}
> local  = {..., 'st_ino': 122420317488, ...}
>
> 122420317488 = 0x1C80CE9BB0
> 2161233200  =   0x80CE9BB0
>
> The field is 4 bytes wide in the protocol struct, while st_ino is 64-bit on Linux:
>
> gdbsupport/fileio.h:130   fio_uint_t  fst_ino;
> gdbsupport/fileio.cc:281  host_to_fileio_uint ((long) st->st_ino, fst->fst_ino);
>
> fst_size, fst_blksize and fst_blocks in the same struct already use the 8-byte fio_ulong_t.
> It reproduces on xfs (86 TB, largest inode 171801755018) and passes on ext4 (2 TB, largest inode 132907009),
> so it depends on the filesystem rather than the architecture.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=34567 <https://sourceware.org/bugzilla/show_bug.cgi?id=34567>
>
> Before writing anything I wanted to ask how you'd prefer this handled.
>
> struct fio_stat is shared between the vFile packets and the older F-packet protocol,
> so widening fst_ino changes the wire layout for both.
>
> I can see three options:
>
>   1. Widen the shared field.

I think this would not be accepted as this would break communication
between different versions of GDB and gdbserver, right?

>   2. Add a separate wider struct used only by the vFile packets,
>      along the lines of your reasoning in c29a37f7417 about those packetsstill being new.

I think only fixing vFile would be a mistake.  The 'F' packets are not
an older protocol that has been replaced with vFile.  The two systems
offer similar functionality, but in opposite directions.  If we're
fixing one direction then we really should fix both.

In c29a37f7417 I changed the underlying implementation of the 'stat'
packet from using 'lstat' to using 'stat'.  The actual on-the-wire bits
didn't change, just what gdbserver did with them.  An old GDB can still
talk to a new gdbserver and vice versa.  What you're proposing would
break this cross version communication, just like option #1, right?

>   3. Negotiate the wider format through qSupported.

I think this is the only possible way forward unfortunately.  It is
going to be more work, but anything else is going to end up breaking
backward compatibility.

>
> The second seems the least disruptive, but I don't have a good sense of who else implements these packets.

That's a huge problem we have.  We really have no visibility at all for
how this stuff is used outside the GDB project.  We solve this problem
by basically assuming that anything that has been released might be
being used, and so cannot be changed.  That's probably not true, but we
just have no way of knowing.

>
> Also worth deciding at the same time, if the layout is being revised:
> fst_dev and fst_rdev are fio_uint_t although dev_t is 64-bit, and the three timestamps are 4 bytes.

It sounds like what you're saying is that a whole bunch of fields are
the wrong size.  I put together this table:

  | Field Name  | Type        | Size | Actual |
  |             |             |  Now | Size   |
  |-------------+-------------+------+--------|
  | fst_dev     | fio_uint_t  |    4 | 8      |
  | fst_ino     | fio_uint_t  |    4 | 8      |
  | fst_mode    | fio_mode_t  |    4 | ?      |
  | fst_nlink   | fio_uint_t  |    4 | ?      |
  | fst_uid     | fio_uint_t  |    4 | ?      |
  | fst_gid     | fio_uint_t  |    4 | ?      |
  | fst_rdev    | fio_uint_t  |    4 | 8      |
  | fst_size    | fio_ulong_t |    8 | ?      |
  | fst_blksize | fio_ulong_t |    8 | ?      |
  | fst_blocks  | fio_ulong_t |    8 | ?      |
  | fst_atime   | fio_time_t  |    4 | 8?     |
  | fst_mtime   | fio_time_t  |    4 | 8?     |
  | fst_ctime   | fio_time_t  |    4 | 8?     |

The 'Size Now' is the current field size in fio_stat, while the 'Actual
Size' is what the fields need to be in order to be correct on your
system.  You mention the three timestamps above, but don't say what size
they need to be.  I'm assuming 8, but that might not be correct either.
Also there are some fields that you haven't mentioned, maybe they are
all correct, but we should check.

What I'd suggest is that you finish filling in the above table, then I
think you'll want to have two versions of fio_stat, one with the current
field sizes, and one with the wider field sizes.

I'd suggest that you update the core throughout to use the struct with
the wider field sizes, but at some point before this struct is sent down
the wire, if the use of the wide struct has not been negotiated, then
you'll need to squeeze the wide struct into the short one, trimming the
fields.  I think it's OK to print a warning if non-zero information is
discarded at this point.

Similarly, when one of these structs is pulled from the wire, if the
incoming packet is the narrow version, I'd expand it into the wide
version.  In this way, most GDB/gdbserver code will only need to handle
the version with the wider fields, but the wire protocol remains
unchanged.

Thanks,
Andrew