[Pvfs2-developers] Review: readcaching code
Sam Lang
slang at mcs.anl.gov
Mon Aug 20 18:34:07 EDT 2007
I went ahead and just changed the PVFS_*_FL flags to match the one's
specified by FS_*_FL in fs.h. I've also tried to address your other
concerns, I think the SETFLAGS was a bit broken -- I need to do a
get_user to get the actual flags value passed in.
-sam
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ioctl-chattr2.patch
Type: application/octet-stream
Size: 3165 bytes
Desc: not available
Url : http://www.beowulf-underground.org/pipermail/pvfs2-developers/attachments/20070820/996369d4/ioctl-chattr2.obj
-------------- next part --------------
On Aug 20, 2007, at 1:27 AM, Murali Vilayannur wrote:
> Hey Sam,
>> Actually pvfs2_xattr_get_default calls pvfs2_inode_getxattr which
>> returns the size of the extended attribute.
>
> Oh right.. Only set_default returns 0 or -ve..
> Good..
>>
>>> - This looks incorrect:
>>>
>>> if(ret >= 0)
>>> + {
>>> + return put_user(val, (int __user *)arg);
>>> + }
>>
>> I pulled that out of the ext3 ioctl code. Seems to work. Without it
>> lsattr gives bad results.
>
> What I meant was not the put_user() part :)
> What I meant was it may be incorrect to put the value of "val" to
> user-space
> due to
>
>>> Flags:
>>> PVFS_IMMUTABLE_FL != FS_IMMUTABLE_FL
>>> PVFS_APPEND_FL != FS_APPEND_FL
>>> PVFS_NOATIME_FL != FS_NOATIME_FL
>
> the above inequalities.. hence "Val" needs to be converted before the
> put_user().
>
>>
>> Yeah, why don't they?
>
> I did not realize we were going to integrate them with
> FS_IOC_SETFLAGS...
> at that time :)
> If the values used by the kernel are unused for our flags, by all
> means they can be changed.
> Has there been any release for folks making use of the current value
> of flags since we store them on disk..? :(
>>
>>>
>>> Before the put_user(), you should convert val from a PVFS_*_FL to a
>>> FS_*_FL flag I think.
>>> ELse the chattr utility won't understand these flags..
>
> This is what I hinted at above. Not the put_user() being a mistake..
>
>>> - if(arg & FS_APPEND_FL)
>>> + {
>>> + val |= PVFS_IMMUTABLE_FL; <--- PVFS_APPEND_FL
>>> + }
>
>
>>>
>>> - should XATTR_CREATE simply be 0?
>>> XATTR_CREATE will fail if a similar xattr already exists I think.
>>
>> Ok. What does 0 do if one doesn't already exist? There's
>> XATTR_REPLACE too which suggests that you either need one or the
>> other.
>
> 0 (default) does the right thing. Create if it does not exist,
> overwrite if they do.
> REPLACE fails if it does not exist, CREATE fails if it does.
>
>>
>> No idea. Its tested on x86_64 with 64 bit userspace.
>
> Okay.. never mind. this is not important :)
> thanks
> Murali
>>
>> -sam
>>>
>>> thanks,
>>> Murali
>>>
>>>>
>>>> -sam
>>>>
>>>>
>>>>
>>>>
>>>>> thanks,
>>>>> Murali
>>>>>
>>>>>> Maybe I'm missing something but I think deleting the file
>>>>>> should be
>>>>>> allowed, in fact it should be the only way to remove the
>>>>>> immutable
>>>>>> attribute.
>>>>>>
>>>>>> -sam
>>>>>>
>>>>>>> thanks,
>>>>>>> Murali
>>>>>>>>
>>>>>>>> -sam
>>>>>>>>
>>>>>>>> On Aug 17, 2007, at 10:09 PM, Murali Vilayannur wrote:
>>>>>>>>
>>>>>>>>> Sam,
>>>>>>>>> The problem is not in the system call (fsetxattr) but the
>>>>>>>>> arguments
>>>>>>>>> to it..
>>>>>>>>> user.pvfs2.meta_hint is the key and val is actually a uint64
>>>>>>>>> which is
>>>>>>>>> a bitwise OR
>>>>>>>>> of PVFS_IMMUTABLE_FL, other pvfs flags.
>>>>>>>>> modify_val() in pvfs2-xattr.c will give an example of this
>>>>>>>>> usage.
>>>>>>>>> Sorry, it is a little convoluted ..:(
>>>>>>>>> but I couldn't/didn't want to do more string parsing on server
>>>>>>>>> side.
>>>>>>>>> Feel free to change that if you think it is needlessly
>>>>>>>>> convoluted.
>>>>>>>>> thanks,
>>>>>>>>> Murali
>>>>>>>>>
>>>>>>>>> PS: let me know how the caching patches work out :)
>>>>>>>>> I havent had too much time to play with it since Feb though.
>>>>>>>>> Hope it works :)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8/17/07, Sam Lang <slang at mcs.anl.gov> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Murali,
>>>>>>>>>>
>>>>>>>>>> I wrote a little program to test the performance of the read-
>>>>>>>>>> caching
>>>>>>>>>> immutable file stuff. With the attached program, I get a
>>>>>>>>>> EINVAL
>>>>>>>>>> error on the read of the file after the immutable
>>>>>>>>>> attribute has
>>>>>>>>>> been
>>>>>>>>>> set (using fsetxattr). Also, ls -la gives me really strange
>>>>>>>>>> results
>>>>>>>>>> for the files that I've set that immutable attribute on. In
>>>>>>>>>> the
>>>>>>>>>> below listing, tmpfile1 and tmpfile10 didn't have the
>>>>>>>>>> immutable
>>>>>>>>>> attribute set. It looks like the problem is with the
>>>>>>>>>> fsetxattr
>>>>>>>>>> system call. The setfattr util does the same thing. When I
>>>>>>>>>> set
>>>>>>>>>> the
>>>>>>>>>> xattr with pvfs2-xattr though, I don't see the corruption in
>>>>>>>>>> listing
>>>>>>>>>> the file. I'll try to investigate what fsetxattr is doing,
>>>>>>>>>> but are
>>>>>>>>>> you aware of any problems with using the system call?
>>>>>>>>>>
>>>>>>>>>> -sam
>>>>>>>>>>
>>>>>>>>>> root at bb22:/tmp/pvfsmnt# ls -la
>>>>>>>>>> total 10260
>>>>>>>>>> drwxrwxrwt 1 slang mpi 4096 2007-08-17 16:35 .
>>>>>>>>>> drwxrwxrwt 5 root root 4096 2007-08-17 15:47 ..
>>>>>>>>>> drwxrwxrwx 1 slang mpi 4096 2007-08-17 15:47 lost+found
>>>>>>>>>> -rw-r--r-- 1 root root 0 2007-08-17 16:24 tmpfile1
>>>>>>>>>> -rw-r--r-- 1 root root 10485760 2007-08-17 16:34 tmpfile10
>>>>>>>>>> ?--------- ? ? ? ? ? tmpfile11
>>>>>>>>>> ?--------- ? ? ? ? ? tmpfile2
>>>>>>>>>> ?--------- ? ? ? ? ? tmpfile3
>>>>>>>>>> ?--------- ? ? ? ? ? tmpfile4
>>>>>>>>>> ?--------- ? ? ? ? ? tmpfile5
>>>>>>>>>> ?--------- ? ? ? ? ? tmpfile6
>>>>>>>>>> ?--------- ? ? ? ? ? tmpfile7
>>>>>>>>>> ?--------- ? ? ? ? ? tmpfile9
>>>>>>>>>> root at bb22:/tmp/pvfsmnt#
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Feb 20, 2007, at 1:06 AM, Murali Vilayannur wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi all,
>>>>>>>>>>> Finally, I got some time to whip up the read-caching
>>>>>>>>>>> patches for
>>>>>>>>>>> non-mutable files into a semblance of shape and stability.
>>>>>>>>>>> With this patch, I am able to get I/Os to a file (marked
>>>>>>>>>>> immutable)
>>>>>>>>>>> serviced from the page-cache. One can tag a file as
>>>>>>>>>>> immutable by
>>>>>>>>>>> running,
>>>>>>>>>>> ./src/apps/admin/pvfs2-xattr -s -k user.pvfs2.meta_hint -v
>>>>>>>>>>> "+immutable" /path/to/pvfs2-file
>>>>>>>>>>> To verify if a file is indeed tagged immutable,
>>>>>>>>>>> ./src/apps/admin/pvfs2-xattr -t -k user.pvfs2.meta_hint /
>>>>>>>>>>> path/
>>>>>>>>>>> to/
>>>>>>>>>>> pvfs2-file
>>>>>>>>>>> (or)
>>>>>>>>>>> ./src/apps/admin/pvfs2-stat /path/to/pvfs2/file
>>>>>>>>>>>
>>>>>>>>>>> I have also added some preliminary statistics exported via
>>>>>>>>>>> /proc/sys/pvfs2/stats/
>>>>>>>>>>> that can be used as a placeholder for more interesting
>>>>>>>>>>> statistics
>>>>>>>>>>> later on.
>>>>>>>>>>> Currently, it only shows # of reads, writes, hits in
>>>>>>>>>>> thepage-
>>>>>>>>>>> cache
>>>>>>>>>>> and misses.
>>>>>>>>>>>
>>>>>>>>>>> For some reason now, cache hits do not happen across a file
>>>>>>>>>>> close.
>>>>>>>>>>> Within a file open-close session, all reads get serviced
>>>>>>>>>>> from
>>>>>>>>>>> the
>>>>>>>>>>> cache though. Very weird.
>>>>>>>>>>> My hunch is that file pages are somehow getting removed
>>>>>>>>>>> from the
>>>>>>>>>>> radix
>>>>>>>>>>> tree of the address space due to some page-ref counting
>>>>>>>>>>> issues. I
>>>>>>>>>>> will
>>>>>>>>>>> dig into this later this week.
>>>>>>>>>>>
>>>>>>>>>>> In any case, this code should not cause any regression of
>>>>>>>>>>> older
>>>>>>>>>>> code
>>>>>>>>>>> paths (hopefully!) and should not impose any performance
>>>>>>>>>>> penalties for
>>>>>>>>>>> workloads making use of the page-cache because of the way we
>>>>>>>>>>> aggregate
>>>>>>>>>>> cache miss I/Os to the server.
>>>>>>>>>>> It was really nice to be able to make use of the iox()
>>>>>>>>>>> infrastructure
>>>>>>>>>>> that was already in place to service non-contigous file and
>>>>>>>>>>> memory
>>>>>>>>>>> I/O.
>>>>>>>>>>> More details of the implementation is described in the
>>>>>>>>>>> thread
>>>>>>>>>>> below.
>>>>>>>>>>> http://www.beowulf-underground.org/pipermail/pvfs2-
>>>>>>>>>>> developers/
>>>>>>>>>>> 2006-
>>>>>>>>>>> November/002847.html
>>>>>>>>>>> Hopefully, I have addressed most of Pete's comments.
>>>>>>>>>>> More comments and testing welcome!
>>>>>>>>>>> thanks,
>>>>>>>>>>> Murali
>>>>>>>>>>> <read-cache-5.patch>
>>>>>>>>>>> _______________________________________________
>>>>>>>>>>> Pvfs2-developers mailing list
>>>>>>>>>>> Pvfs2-developers at beowulf-underground.org
>>>>>>>>>>> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-
>>>>>>>>>>> developers
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>
More information about the Pvfs2-developers
mailing list