On Wed, 2017-01-25 at 08:58 +1100, NeilBrown wrote: > On Mon, Jan 23 2017, Jeff Layton wrote: > > > On Tue, 2017-01-24 at 11:16 +1100, NeilBrown wrote: > > > On Mon, Jan 23 2017, Trond Myklebust wrote: > > > > > > > On Mon, 2017-01-23 at 17:35 -0500, Jeff Layton wrote: > > > > > On Mon, 2017-01-23 at 11:09 +0100, Kevin Wolf wrote: > > > > > > > > > > > > However, if we look at the greater problem of hanging requests that > > > > > > came > > > > > > up in the more recent emails of this thread, it is only moved > > > > > > rather > > > > > > than solved. Chances are that already write() would hang now > > > > > > instead of > > > > > > only fsync(), but we still have a hard time dealing with this. > > > > > > > > > > > > > > > > Well, it _is_ better with O_DIRECT as you can usually at least break > > > > > out > > > > > of the I/O with SIGKILL. > > > > > > > > > > When I last looked at this, the problem with buffered I/O was that > > > > > you > > > > > often end up waiting on page bits to clear (usually PG_writeback or > > > > > PG_dirty), in non-killable sleeps for the most part. > > > > > > > > > > Maybe the fix here is as simple as changing that? > > > > > > > > At the risk of kicking off another O_PONIES discussion: Add an > > > > open(O_TIMEOUT) flag that would let the kernel know that the > > > > application is prepared to handle timeouts from operations such as > > > > read(), write() and fsync(), then add an ioctl() or syscall to allow > > > > said application to set the timeout value. > > > > > > I was thinking on very similar lines, though I'd use 'fcntl()' if > > > possible because it would be a per-"file description" option. > > > This would be a function of the page cache, and a filesystem wouldn't > > > need to know about it at all. Once enable, 'read', 'write', or 'fsync' > > > would return EWOULDBLOCK rather than waiting indefinitely. > > > It might be nice if 'select' could then be used on page-cache file > > > descriptors, but I think that is much harder. Support O_TIMEOUT would > > > be a practical first step - if someone agreed to actually try to use it. > > > > > > > Yeah, that does seem like it might be worth exploring. > > > > That said, I think there's something even simpler we can do to make > > things better for a lot of cases, and it may even help pave the way for > > the proposal above. > > > > Looking closer and remembering more, I think the main problem area when > > the pages are stuck in writeback is the wait_on_page_writeback call in > > places like wait_for_stable_page and __filemap_fdatawait_range. > > I can't see wait_for_stable_page() being very relevant. That only > blocks on backing devices which have requested stable pages. > raid5 sometimes does that. Some scsi/sata devices can somehow. > And rbd (part of ceph) sometimes does. I don't think NFS ever will. > wait_for_stable_page() doesn't currently return an error, so getting to > abort in SIGKILL would be a lot of work. > Ahh right, I missed that it only affects pages backed by a BDI that has BDI_CAP_STABLE_WRITES. Good. > filemap_fdatawait_range() is much easier. > > diff --git a/mm/filemap.c b/mm/filemap.c > index b772a33ef640..2773f6dde1da 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -401,7 +401,9 @@ static int __filemap_fdatawait_range(struct address_space *mapping, > if (page->index > end) > continue; > > - wait_on_page_writeback(page); > + if (PageWriteback(page)) > + if (wait_on_page_bit_killable(page, PG_writeback)) > + err = -ERESTARTSYS; > if (TestClearPageError(page)) > ret = -EIO; > } > > That isn't a complete solution. There is code in f2fs which doesn't > check the return value and probably should. And gfs2 calls > mapping_set_error(mapping, error); > with the return value, with we probably don't want in the ERESTARTSYS case. > There are some usages in btrfs that I'd need to double-check too. > > But it looks to be manageable. > > Thanks, > NeilBrown > Yeah, it does. The main worry I have is that this function is called all over the place in fairly deep call chains. It definitely needs review and testing (and probably a lot of related fixes like you mention). We should also note that this is not really a fix for applications, per-se. It's more of an administrative improvement, to allow admins to kill off processes stuck waiting for an inode to finish writeback. I think there may still be room for an interface like you and Trond were debating. But, I think this might be a good first step and would improve a lot of hung mount situations. -- Jeff Layton <jlayton@xxxxxxxxxxxxxxx> -- 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
