Miklos Szeredi <miklos@xxxxxxxxxx> writes:
>>> + info = SHMEM_I(dentry->d_inode);
>>> +
>>> + spin_lock(&dentry->d_inode->i_lock);
>>
>> Not important, but I suggest you use info->lock throughout for this,
>> instead of dentry->d_inode->i_lock: in each case you need "info" for
>> info->xattr_list (and don't need "inode" at all I think), so info->lock
>> seems appropriate, and may be in the same cacheline once I make
>> shmem_inode_info smaller. But don't worry if you'd prefer to leave
>> it.
>
> Makes sense. I updated the locking.
This uncovered a nasty bug lurking in there: the "info" area, including
lock and xattr_list, may be overwritten by inline symlinks. Because
xattr_list is near the end, this wasn't noticed with casual testing, but
info->lock would immediately Oops on getxattr for symlinks.
I propose the following solution. It results in slightly less space for
inline symlinks, but correct operation for xattrs. Does the anonymous
union/struct solution look acceptable?
Thanks,
Miklos
Index: linux-2.6/include/linux/shmem_fs.h
===================================================================
--- linux-2.6.orig/include/linux/shmem_fs.h 2011-05-12 15:59:08.000000000 +0200
+++ linux-2.6/include/linux/shmem_fs.h 2011-05-12 15:58:25.000000000 +0200
@@ -11,15 +11,39 @@
struct shmem_inode_info {
spinlock_t lock;
- unsigned long flags;
- unsigned long alloced; /* data pages alloced to file */
- unsigned long swapped; /* subtotal assigned to swap */
- unsigned long next_index; /* highest alloced index + 1 */
- struct shared_policy policy; /* NUMA memory alloc policy */
- struct page *i_indirect; /* top indirect blocks page */
- swp_entry_t i_direct[SHMEM_NR_DIRECT]; /* first blocks */
- struct list_head swaplist; /* chain of maybes on swap */
- struct list_head xattr_list; /* list of shmem_xattr */
+
+ /* list of shmem_xattr */
+ struct list_head xattr_list;
+
+ union {
+ char inline_symlink[0];
+
+ /* Members not used by inline symlinks: */
+ struct {
+ unsigned long flags;
+
+ /* data pages alloced to file */
+ unsigned long alloced;
+
+ /* subtotal assigned to swap */
+ unsigned long swapped;
+
+ /* highest alloced index + 1 */
+ unsigned long next_index;
+
+ /* NUMA memory alloc policy */
+ struct shared_policy policy;
+
+ /* top indirect blocks page */
+ struct page *i_indirect;
+
+ /* first blocks */
+ swp_entry_t i_direct[SHMEM_NR_DIRECT];
+
+ /* chain of maybes on swap */
+ struct list_head swaplist;
+ };
+ };
struct inode vfs_inode;
};
Index: linux-2.6/mm/shmem.c
===================================================================
--- linux-2.6.orig/mm/shmem.c 2011-05-12 15:59:08.000000000 +0200
+++ linux-2.6/mm/shmem.c 2011-05-12 15:50:10.000000000 +0200
@@ -2029,9 +2029,9 @@ static int shmem_symlink(struct inode *d
info = SHMEM_I(inode);
inode->i_size = len-1;
- if (len <= (char *)inode - (char *)info) {
+ if (len <= (char *)inode - info->inline_symlink) {
/* do it inline */
- memcpy(info, symname, len);
+ memcpy(info->inline_symlink, symname, len);
inode->i_op = &shmem_symlink_inline_operations;
} else {
error = shmem_getpage(inode, 0, &page, SGP_WRITE, NULL);
@@ -2057,7 +2057,7 @@ static int shmem_symlink(struct inode *d
static void *shmem_follow_link_inline(struct dentry *dentry, struct nameidata *nd)
{
- nd_set_link(nd, (char *)SHMEM_I(dentry->d_inode));
+ nd_set_link(nd, SHMEM_I(dentry->d_inode)->inline_symlink);
return NULL;
}
--
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
