On Thu, Apr 14, 2016 at 1:37 PM, Theodore Ts'o <tytso@xxxxxxx> wrote:
>
> So Linus's proposal to
> add "if (signal_pending(current)) return -EINTR;" to filldir64() would
> probably cause more than a few userspace regressions.
I don't think you actually read or understood my proposal.
The proposal added it to inside the if-statement in
dirent = buf->previous;
if (dirent) {
+ if (signal_pending(current)) return -EINTR;
and that actually guarantees that readdir() _never_ returns -EINTR,
because there is at least one entry that got filled out (the previous
one filled in, now pointed to be "dirent").
So the iterator wrapper that is actually readdir() (getdents) will see
that filldir64 returned an error, and stop filling entries. But
because at least entry has been filled, it will return a positive
number - the amount filled.
This is similar to the interruptible partial read or write case:
getting a signal after something has been read or written will not
return -EINTR, it will return the partial result.
And we _know_ that getdents is ok with partial results, since the
caller by definition doesn't know how many entries it will get, and
you generally never get a completely full buffer anyway (since
directory entries have varying sizes and the last one seldom fits
exactly in the buffer provided).
I ran that kernel for several days, in addition to testing it with the
test-program that broke with the ext4 changes. It was fine. I didn't
commit it, because I didn't think it was appropriate for outside the
merge window, but I don't believe it can break.
Now, it is very possible that ext4 might want to handle the fatal
signal despite that, simply because ext4 may be looping over blocks
that simply don't contain any directory entries at all. That said, it
is possible that my one-liner makes your fatal-signal case irrelevant,
simply because it makes it so unlikely to matter in practice.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
