Re: stampede hd0*

[Thomas Bushnell, BSG]

Roland McGrath <address@hidden> writes:

> I see two problems in diskfs and ext2fs in their handling of the S_IPTRANS
> bit.  But it is not entirely clear what the interface guarantees are
> intended to be.  

Ah, but I know what they are intended to be!  :)

> First, diskfs_S_file_chmod implicitly clears both S_ITRANS
> bits from dn_stat.st_mode, which I think is wrong.  

chmod is not supposed to be able to affect the IATRANS and IPTRANS
bits, for the same reason it is not able to affect the IFMT bits.

> I guess the S_IATRANS bit in dn_stat.st_mode never matters one way
> or the other (diskfs_S_io_stat sets it in the result as needed), so
> I don't know whether it is supposed to be set or not.

Correct.  Nothing should check that bit internally, and it should
never be written on disk.

> The S_IPTRANS bit in dn_stat.st_mode communicates the translatedness
> state between diskfs and the filesystem back end.  

Correct.  The back end sets it if there is a translator; but it's
purely a one-way communications channel, from the back end to diskfs.  

> It's not clear to me whether clearing that in dn_stat.st_mode and
> then calling diskfs_write_disknode is supposed to clear the
> translator setting on disk.

Doing that should never clear the translator setting on disk.  The
only way to do that is diskfs_set_translator.

> That is what the ext2fs code does, but it doesn't do all the bookkeeping
> properly if that happens without diskfs_set_translator having been called
> to clear it.  The ufs code doesn't examine S_IPTRANS at all

The ext2fs write_node function is incorrect.  I assume that Miles
misunderstood when he cloned the ufs function.

> So I think that ext2fs is incorrect to test S_IPTRANS in write_node.

Correct.

> However, that test can never mismatch the existing i_translator setting if
> diskfs_set_translator was called.  And I think diskfs is incorrect for
> clearing S_IPTRANS in dn_stat.st_mode before calling write_node.

We should never set IPTRANS (or IATRANS) on disk, in my opinion,
simply for the reason that whatever values are found on disk should be
entirely ignored.  But that's really the bottom half's business.

> I am surmising that the protocol is, or should be, that S_IPTRANS is never
> modified in dn_stat.st_mode by diskfs, and the diskfs_set_translator
> callback is the only thing that should ever change it.  

That's right.

> If that's right, the following patch to diskfs makes it preserve the
> bit during file_chmod.

file-chmod.c already takes steps to preserve IFMT and ISPARE, while
preventing the user from modifying them, with the line:
    mode |= (np->dn_stat.st_mode & (S_IFMT | S_ISPARE));

A better patch than yours, IMO, is to a S_ITRANS to that, so that it
parallels the first line of the function exactly.

> With that, the ext2fs code that tests S_IPTRANS will never change anything
> from how it is, and so I am inclined to remove it just for clarity.

Indeed, it should be removed.

Thomas