Skip to content

Commit 88a2f64

Browse files
author
Al Viro
committed
struct fd: representation change
We want the compiler to see that fdput() on empty instance is a no-op. The emptiness check is that file reference is NULL, while fdput() is "fput() if FDPUT_FPUT is present in flags". The reason why fdput() on empty instance is a no-op is something compiler can't see - it's that we never generate instances with NULL file reference combined with non-zero flags. It's not that hard to deal with - the real primitives behind fdget() et.al. are returning an unsigned long value, unpacked by (inlined) __to_fd() into the current struct file * + int. The lower bits are used to store flags, while the rest encodes the pointer. Linus suggested that keeping this unsigned long around with the extractions done by inlined accessors should generate a sane code and that turns out to be the case. Namely, turning struct fd into a struct-wrapped unsinged long, with fd_empty(f) => unlikely(f.word == 0) fd_file(f) => (struct file *)(f.word & ~3) fdput(f) => if (f.word & 1) fput(fd_file(f)) ends up with compiler doing the right thing. The cost is the patch footprint, of course - we need to switch f.file to fd_file(f) all over the tree, and it's not doable with simple search and replace; there are false positives, etc. Note that the sole member of that structure is an opaque unsigned long - all accesses should be done via wrappers and I don't want to use a name that would invite manual casts to file pointers, etc. The value of that member is equal either to (unsigned long)p | flags, p being an address of some struct file instance, or to 0 for an empty fd. For now the new predicate (fd_empty(f)) has no users; all the existing checks have form (!fd_file(f)). We will convert to fd_empty() use later; here we only define it (and tell the compiler that it's unlikely to return true). This commit only deals with representation change; there will be followups. Reviewed-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
1 parent 1da91ea commit 88a2f64

File tree

6 files changed

+35
-23
lines changed

6 files changed

+35
-23
lines changed

drivers/infiniband/core/uverbs_cmd.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -572,7 +572,7 @@ static int ib_uverbs_open_xrcd(struct uverbs_attr_bundle *attrs)
572572
struct inode *inode = NULL;
573573
int new_xrcd = 0;
574574
struct ib_device *ib_dev;
575-
struct fd f = {};
575+
struct fd f = EMPTY_FD;
576576
int ret;
577577

578578
ret = uverbs_request(attrs, &cmd, sizeof(cmd));

fs/overlayfs/file.c

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -93,11 +93,11 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
9393
bool allow_meta)
9494
{
9595
struct dentry *dentry = file_dentry(file);
96+
struct file *realfile = file->private_data;
9697
struct path realpath;
9798
int err;
9899

99-
real->flags = 0;
100-
real->file = file->private_data;
100+
real->word = (unsigned long)realfile;
101101

102102
if (allow_meta) {
103103
ovl_path_real(dentry, &realpath);
@@ -113,27 +113,29 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
113113
return -EIO;
114114

115115
/* Has it been copied up since we'd opened it? */
116-
if (unlikely(file_inode(real->file) != d_inode(realpath.dentry))) {
117-
real->flags = FDPUT_FPUT;
118-
real->file = ovl_open_realfile(file, &realpath);
119-
120-
return PTR_ERR_OR_ZERO(real->file);
116+
if (unlikely(file_inode(realfile) != d_inode(realpath.dentry))) {
117+
struct file *f = ovl_open_realfile(file, &realpath);
118+
if (IS_ERR(f))
119+
return PTR_ERR(f);
120+
real->word = (unsigned long)ovl_open_realfile(file, &realpath) | FDPUT_FPUT;
121+
return 0;
121122
}
122123

123124
/* Did the flags change since open? */
124-
if (unlikely((file->f_flags ^ real->file->f_flags) & ~OVL_OPEN_FLAGS))
125-
return ovl_change_flags(real->file, file->f_flags);
125+
if (unlikely((file->f_flags ^ realfile->f_flags) & ~OVL_OPEN_FLAGS))
126+
return ovl_change_flags(realfile, file->f_flags);
126127

127128
return 0;
128129
}
129130

130131
static int ovl_real_fdget(const struct file *file, struct fd *real)
131132
{
132133
if (d_is_dir(file_dentry(file))) {
133-
real->flags = 0;
134-
real->file = ovl_dir_real_file(file, false);
135-
136-
return PTR_ERR_OR_ZERO(real->file);
134+
struct file *f = ovl_dir_real_file(file, false);
135+
if (IS_ERR(f))
136+
return PTR_ERR(f);
137+
real->word = (unsigned long)f;
138+
return 0;
137139
}
138140

139141
return ovl_real_fdget_meta(file, real, false);

fs/xfs/xfs_handle.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ xfs_find_handle(
8585
int hsize;
8686
xfs_handle_t handle;
8787
struct inode *inode;
88-
struct fd f = {NULL};
88+
struct fd f = EMPTY_FD;
8989
struct path path;
9090
int error;
9191
struct xfs_inode *ip;

include/linux/file.h

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,18 +35,28 @@ static inline void fput_light(struct file *file, int fput_needed)
3535
fput(file);
3636
}
3737

38+
/* either a reference to struct file + flags
39+
* (cloned vs. borrowed, pos locked), with
40+
* flags stored in lower bits of value,
41+
* or empty (represented by 0).
42+
*/
3843
struct fd {
39-
struct file *file;
40-
unsigned int flags;
44+
unsigned long word;
4145
};
4246
#define FDPUT_FPUT 1
4347
#define FDPUT_POS_UNLOCK 2
4448

45-
#define fd_file(f) ((f).file)
49+
#define fd_file(f) ((struct file *)((f).word & ~(FDPUT_FPUT|FDPUT_POS_UNLOCK)))
50+
static inline bool fd_empty(struct fd f)
51+
{
52+
return unlikely(!f.word);
53+
}
54+
55+
#define EMPTY_FD (struct fd){0}
4656

4757
static inline void fdput(struct fd fd)
4858
{
49-
if (fd.flags & FDPUT_FPUT)
59+
if (fd.word & FDPUT_FPUT)
5060
fput(fd_file(fd));
5161
}
5262

@@ -60,7 +70,7 @@ extern void __f_unlock_pos(struct file *);
6070

6171
static inline struct fd __to_fd(unsigned long v)
6272
{
63-
return (struct fd){(struct file *)(v & ~3),v & 3};
73+
return (struct fd){v};
6474
}
6575

6676
static inline struct fd fdget(unsigned int fd)
@@ -80,7 +90,7 @@ static inline struct fd fdget_pos(int fd)
8090

8191
static inline void fdput_pos(struct fd f)
8292
{
83-
if (f.flags & FDPUT_POS_UNLOCK)
93+
if (f.word & FDPUT_POS_UNLOCK)
8494
__f_unlock_pos(fd_file(f));
8595
fdput(f);
8696
}

kernel/events/core.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12474,7 +12474,7 @@ SYSCALL_DEFINE5(perf_event_open,
1247412474
struct perf_event_attr attr;
1247512475
struct perf_event_context *ctx;
1247612476
struct file *event_file = NULL;
12477-
struct fd group = {NULL, 0};
12477+
struct fd group = EMPTY_FD;
1247812478
struct task_struct *task = NULL;
1247912479
struct pmu *pmu;
1248012480
int event_fd;

net/socket.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -559,7 +559,7 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed)
559559
if (fd_file(f)) {
560560
sock = sock_from_file(fd_file(f));
561561
if (likely(sock)) {
562-
*fput_needed = f.flags & FDPUT_FPUT;
562+
*fput_needed = f.word & FDPUT_FPUT;
563563
return sock;
564564
}
565565
*err = -ENOTSOCK;

0 commit comments

Comments
 (0)