On Sat, Apr 9, 2016 at 7:23 PM, Greg Thelen <gthelen@xxxxxxxxxx> wrote:
>
>
> I've been testing 5b5b7fd185e9 (linus/master) and seeing that
> interrupted readdir() now returns duplicate dirents.
Yes, your test-program recreates this beautifully here. Sadly not
while stracing.
Worth adding as a filesystem test to fsstress?
> Reverting commit 1028b55bafb7 ("ext4: allow readdir()'s of large empty
> directories to be interrupted") avoids duplicates.
Ok, so the the "fatal_signal_pending()" case in ext4_readdir() should
be ok, since we're killing the thread at that point.
So I assume it's the ext4_htree_fill_tree() part of the patch.
What I *think* happens is:
- We are perfectly happy to take a break at "call_filldir()" time (if
that returns an error because the buffer was full or whatever), and at
that point, ctx->pos will point to the last entry that we did *not*
successfully fill in.
- but if we take an exception at ext4_htree_fill_tree() time, we end
the loop, and now "ctx->pos" contains the offset of the previous entry
- the one we *successfully* already copied
- so now, when we exit the getdents, we will save the re-start value
at the entry that we already successfully handled.
So I think that commit is entirely bogus.
I think the right choice might be to
(a) revert that patch (or just change the signal_pending() into
fatal_signal_pending())
(b) to get the latency advantage, do something like this:
diff --git a/fs/readdir.c b/fs/readdir.c
index e69ef3b79787..a2244fe9c003 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -248,6 +248,8 @@ static int filldir64(struct dir_context *ctx,
const char *name, int namlen,
return -EINVAL;
dirent = buf->previous;
if (dirent) {
+ if (signal_pending)
+ return -EINTR;
if (__put_user(offset, &dirent->d_off))
goto efault;
}
which returns that error at a point where we can actually take it
(note that we only do this if we have already filled in at least one
entry: "buf->previous" was non-NULL, so we can return EINTR, because
the caller will actually return the length of the result, never the
EINTR error we're returning).
The above patch is *entirely* untested. It might be pure garbage. But
that commit 1028b55bafb7 is _definitely_ broken, and the above _might_
work.
Caveat patchor.
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
