[#LU-787] ftruncate blocks when done holding a group lock

[LU-787] ftruncate blocks when done holding a group lock Created: 23/Oct/11 Updated: 05/Aug/20 Resolved: 28/Sep/12
Status:	Resolved
Project:	Lustre
Component/s:	None
Affects Version/s:	Lustre 2.1.0
Fix Version/s:	Lustre 2.3.0

Type:

Bug

Priority:

Major

Reporter:

CEA

Assignee:

Oleg Drokin

Resolution:

Fixed

Votes:

Labels:

None

Environment:

Nothing special

Attachments:

truncate-gl.patch

Issue Links:

Related
is related to	~~LU-1031~~	Add grouplock support to multiop	Resolved

Severity:

Rank (Obsolete):

4675

Description

After geting a group lock, ftruncate blocks (because of the grouplock) instead of working

Attached a patch which :

modifies multiop to add group lock support
add a sanity test, which fails

Comments

Comment by Peter Jones [ 25/Oct/11 ]

Oleg

Could you please review this patch http://review.whamcloud.com/#change,1640

Thanks

Peter

Comment by Johann Lombardi (Inactive) [ 30/Jan/12 ]

Hi JC,

For read & write, ll_io_init() checks for the per-fd LL_FILE_IGNORE_LOCK flag and set io->ci_lockreq = CILR_NEVER in order to skip locking for this file. Isn't the problem just that there is no equivalent of this code for truncate (i can't find any)?

Comment by jacques-charles lafoucriere [ 30/Jan/12 ]

Hi
I do not find where LL_FILE_IGNORE_LOCK is set, it looks like an external flag settable through ioctl() like grouplock.
ll_file_nolock(), which does the test, is also using the LL_SBI_NOLCK flags criteria which is made for the nolock mount option (so related to posix flock mechanism).
There is another flag LL_FILE_GROUP_LOCKED for group lock which we can be used to set io->ci_lockreq = CILR_NEVER if we already have the right grouplock id (if not we have to block).
Do you want I explore the use of CILR_NEVER in truncate ?

Comment by Johann Lombardi (Inactive) [ 30/Jan/12 ]

> I do not find where LL_FILE_IGNORE_LOCK is set, it looks like an external flag settable through ioctl() like grouplock.

Ah, right. I was actually looking at an old commit which used to manage grouplock as in 1.8. However, the patch from bugzilla 20529 has totally modified the logic, as explained in this comment by Oleg:
https://bugzilla.lustre.org/show_bug.cgi?id=20529#c14

> ll_file_nolock(), which does the test, is also using the LL_SBI_NOLCK flags criteria which is made for the nolock mount option (so related to posix flock mechanism).

Right, the grouplock used to use the same mechanism as -o nolock to bypass locking. This makes me wonder how -o nolock can work now if the LL_FILE_IGNORE_LOCK flag isn't set anywhere ...

> There is another flag LL_FILE_GROUP_LOCKED for group lock which we can be used to set io->ci_lockreq =
> CILR_NEVER if we already have the right grouplock id (if not we have to block).
> Do you want I explore the use of CILR_NEVER in truncate ?

This is how it used to work and this has been changed to "make group locks first class citizens". I guess we need to stick with this approach and forget about using CILR_NEVER for group lock. Let me have another look at your patch then ...

Comment by Johann Lombardi (Inactive) [ 30/Jan/12 ]

hm, ccc_io_one_lock_index() checks for cio->cui_fd (a file descriptor) which is populated in ll_file_io_generic(). I guess that this is what we miss on the truncate path ...

For ftruncate, the VFS is kind enough to pass us a pointer to the struct file via iattr::ia_file:

 462struct iattr {
 463        unsigned int    ia_valid;
 464        umode_t         ia_mode;
 465        uid_t           ia_uid;
 466        gid_t           ia_gid;
 467        loff_t          ia_size;
 468        struct timespec ia_atime;
 469        struct timespec ia_mtime;
 470        struct timespec ia_ctime;
 471
 472        /*
 473         * Not an attribute, but an auxiliary info for filesystems wanting to
 474         * implement an ftruncate() like method.  NOTE: filesystem should
 475         * check for (ia_valid & ATTR_FILE), and not for (ia_file != NULL).
 476         */
 477        struct file     *ia_file;
 478};

Comment by Johann Lombardi (Inactive) [ 02/Feb/12 ]

I have just pushed a tentative - and untested - patch, see http://review.whamcloud.com/2081
I have added JC's regression test so that autotest could tell us whether or not the solution works.

Comment by Johann Lombardi (Inactive) [ 04/Feb/12 ]

It looks like the new patch passed the regression test:
https://maloo.whamcloud.com/sub_tests/99407e26-4eb4-11e1-88dd-5254004bbbd3