All of lore.kernel.org
 help / color / mirror / Atom feed
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?

             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.