From: Al Viro <viro@ZenIV.linux.org.uk>
To: Mike Marshall <hubcap@omnibond.com>
Cc: linux-fsdevel@vger.kernel.org
Subject: [RFC] what's going on with file->f_pos uses in orangefs_file_write_iter()?
Date: Thu, 7 Dec 2017 22:26:10 +0000 [thread overview]
Message-ID: <20171207222610.GH21978@ZenIV.linux.org.uk> (raw)
I'd missed that back then, but...
if (file->f_pos > i_size_read(file->f_mapping->host))
orangefs_i_size_write(file->f_mapping->host, file->f_pos);
rc = generic_write_checks(iocb, iter);
if (rc <= 0) {
gossip_err("%s: generic_write_checks failed, rc:%zd:.\n",
__func__, rc);
goto out;
}
/*
* if we are appending, generic_write_checks would have updated
* pos to the end of the file, so we will wait till now to set
* pos...
*/
pos = *(&iocb->ki_pos);
looks suspicious as hell. What's going on there? Not to mention anything
else file->f_pos might be completely unrelated to any IO going on -
consider e.g. pwrite(2), where the position (in iocb->ki_pos) has nothing
to do with file->f_pos. Then there's the question of WTF is write()
(or pwrite()) past the current EOF doing bumping the file size, before
it even gets a chance to decide whether it'll be trying to do any IO at
all.
_Then_ there's the deadlock on 32bit SMP in that code. Look: several
lines prior we'd done
inode_lock(file->f_mapping->host);
and hadn't unlocked the sucker since then. And
static inline void orangefs_i_size_write(struct inode *inode, loff_t i_size)
{
#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
inode_lock(inode);
#endif
i_size_write(inode, i_size);
#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
inode_unlock(inode);
#endif
}
means that if we get around to calling it there in SMP/32bit case, we'll
get as plain a deadlock as possible. And AFAICS it had been that way
since the initial merge.
What the hell is that code about and what is it trying to do?
PS: While we are at it, what's the point of that *(&...) in there?
next reply other threads:[~2017-12-07 22:26 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-07 22:26 Al Viro [this message]
2017-12-08 16:39 ` [RFC] what's going on with file->f_pos uses in orangefs_file_write_iter()? martin
2017-12-12 16:31 ` Mike Marshall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171207222610.GH21978@ZenIV.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=hubcap@omnibond.com \
--cc=linux-fsdevel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.