On Thu, Dec 22, 2016 at 12:56 AM, Theodore Ts'o <tytso@xxxxxxx> wrote:
> On Wed, Dec 21, 2016 at 06:37:42PM +0200, Amir Goldstein wrote:
>>
>> Right. never claimed that saving data segment space is the issue
>> here. To be completely honest, I started looking at ways to optimize
>> whiteout iteration and was appalled to see all that code copy&paste,
>> so went on an OCD cleanup spree.
>>
>> So it is really a lot more about clumsiness of the current code then
>> about saving actual lines on code or space in data segment.
>>
>> Perhaps I should have settled with defining this common section:
>
>> +#define DT_WHT 14
>
> What are you trying to accomplish here? Unless we actually encoding
> DT_WHT into the on-disk format, it's not really going to buy you much.
>
> And if you are going to encode this into the on-disk format, each file
> system is going to have to set some kind of flag in the superblock to
> indicate whether it's doing this new thing or not. And this is
> *precisely* why Dave and Darrick are objecting --- if we are going to
> make any kind of file system encoding change, the file system has to
> know about it.
>
Ted,
There is a bit of a confusion here.
Although I would have liked to be able to add DT_WHT to dirent
I found there is no east way to do it without adding fs feature flags.
I did not add the DT_WHT definion, I kept it as is just moved
from fs.h to file_type.h because some fs (e.g. xfs) still use this
ghost define.
>> Note that all those file system copy&pasted a potential bug.
>>
>> This is an array of size 15:
>>
>> static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
>>
>> and it is always addressed this way:
>>
>> ext2_type_by_mode[(inode->i_mode & S_IFMT)>>S_SHIFT];
>>
>> Which *can* try to address ext2_type_by_mode[15]
>>
>> Now, can you say for certain that you can never get a malformed i_mode
>> with ((inode->i_mode & S_IFMT) == S_IFMT) either from disk or from
>> some code calling into VFS functions, which do not sanity check the
>> file type of the mode argument.
>
> It's not a problem and not a bug, because we only assign the file type
> when the inode is created, and at that point, the i_mode is set by the
> *kernel*, and not by the on-disk encoding.
>
Not entirley true.
On rename some fs set dentry type of target by converting the mode of
the renamed object.
I don't remember what ext4 does, but I think it does the same,
so malformed imode on disk could potentially get you there.
> And BTW this is why DT_WHT makes no sense. We the DT encodings come
> from the directory entry, and *never* come from the inode i_mode read
> from disk --- since at the time when we do the readdir(2), the inode
> may not have been read into memory. So we can't add DT_WHT unless we
> also change the on-disk encoding of the directory entry on a file
> system by file system basis.
>
True. it makes no sense at all, but we can't remove it.
XFS can decide to start treating it as an error if found on disk,
but that's another story.
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
